Use an IBufferWriter<byte> to write the outgoing SSPI blob #2452

twsouthwick · 2024-04-08T22:19:44Z

This change removes the need to pre-allocate anything for the outgoing blobs of SSPI generation. As part of this:

An internal implementation of ArrayBufferWriter is added for platforms that do not support it
SqlObjectPool is imbued with the ability to create/reset pooled objects
TdsParser/TdsLogin is updated to use pooled ArrayBufferWriter instances to generate SSPI blobs
Native methods are updated to take in Span/* for writeable byte[]
SSPIContextProvider signature is updated to take IBufferWriter

Part of #2253

This change removes the need to pre-allocate anything for the outgoing blobs of SSPI generation. As part of this: - An internal implementation of ArrayBufferWriter is added for platforms that do not support it - SqlObjectPool is imbued with the ability to create/reset pooled objects - TdsParser/TdsLogin is updated to use pooled ArrayBufferWriter instances to generate SSPI blobs - Native methods are updated to take in Span/* for writeable byte[] - SSPIContextProvider signature is updated to take IBufferWriter

twsouthwick · 2024-05-07T21:20:34Z

@David-Engel can I get a review on this next step in the SSPI journey?

codecov · 2024-05-08T03:24:57Z

Codecov Report

Attention: Patch coverage is 78.35821% with 29 lines in your changes missing coverage. Please review.

Project coverage is 66.10%. Comparing base (081fc51) to head (c43f86f).

Files with missing lines	Patch %	Lines
.../src/Microsoft/Data/SqlClient/ArrayBufferWriter.cs	56.25%	21 Missing ⚠️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs	85.41%	7 Missing ⚠️
...t/Data/SqlClient/SSPI/NativeSSPIContextProvider.cs	83.33%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (081fc51) and HEAD (c43f86f). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (081fc51) HEAD (c43f86f)

addons 1 0

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2452      +/-   ##
==========================================
- Coverage   72.73%   66.10%   -6.64%     
==========================================
  Files         283      278       -5     
  Lines       58997    58712     -285     
==========================================
- Hits        42910    38810    -4100     
- Misses      16087    19902    +3815

Flag	Coverage Δ
addons	`?`
netcore	`69.21% <89.15%> (-6.33%)`	⬇️
netfx	`64.88% <77.51%> (-6.32%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

twsouthwick · 2024-05-13T18:04:31Z

@David-Engel ping

David-Engel · 2024-05-13T23:39:20Z

I ran our internal Kerberos test pipeline against this PR and all tests passed.

twsouthwick · 2024-05-28T20:16:11Z

ping @JRahnama @DavoudEshtehari @arellegue

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NegotiateSSPIContextProvider.cs

twsouthwick · 2024-08-19T17:48:42Z

@David-Engel can you run this on the kerberos suite again?

twsouthwick · 2024-08-20T16:50:52Z

@David-Engel something is up with the merges so let me get those fixed before you do the kerberos runs

twsouthwick · 2024-08-20T18:24:10Z

@David-Engel I'm getting sporadic errors that don't seem to always repro: https://sqlclientdrivers.visualstudio.com/public/_build/results?buildId=96359&view=logs&j=b14281d2-3365-502e-c6c8-e9e46d660715&t=a67f9feb-8bad-50ac-92c7-d62292e3e6fb&l=63

Have you seen these or are they related to my pr?

src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj

src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ArrayBufferWriter.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs

DavoudEshtehari · 2024-08-21T02:12:09Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs

@@ -8,6 +8,8 @@ namespace Microsoft.Data.SqlClient
 {
    internal partial class TdsParser
    {
+        private static readonly SqlObjectPool<ArrayBufferWriter<byte>> _writers = new(20, () => new(), a => a.Clear());


No complain here only I'm curious about the constant 20 pooled objects here!
cc @David-Engel

honestly I don't remember where the 20 came from. Seemed reasonable I think at the time, but I'm not really aware of how many concurrent calls to it are called in a given connection

I dug into the SqlObjectPool logic because I was curious, so thought I would share in case anyone else was wondering. 20 here isn't a limit on parallelism or anything. It's just the maximum number that will be cached. Parallel Rents from the pool above 20 will simply get a new object. I agree that 20 should be plenty for the majority of scenarios.

Can you please add a comment in the code indicating the reasoning for 20?

twsouthwick · 2025-01-31T18:46:16Z

@cheenamalhotra can you trigger a build again? will it automatically build if I switch it away from draft?

twsouthwick · 2025-01-31T19:59:30Z

@David-Engel can you kick off the build for this?

David-Engel · 2025-01-31T21:46:38Z

/azp run

azure-pipelines · 2025-01-31T21:46:59Z

Azure Pipelines successfully started running 1 pipeline(s).

twsouthwick · 2025-01-31T22:49:09Z

@David-Engel can you run the tests again?

David-Engel · 2025-01-31T22:50:16Z

/azp run

azure-pipelines · 2025-01-31T22:50:35Z

Azure Pipelines successfully started running 1 pipeline(s).

twsouthwick · 2025-02-01T01:39:12Z

@David-Engel @cheenamalhotra This PR is passing all tests and ready for review. Thanks!

twsouthwick · 2025-02-01T01:40:55Z

...icrosoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.netcore.cs

@@ -842,145 +842,6 @@ internal void WriteByte(byte b)
            _outBuff[_outBytesUsed++] = b;
        }

-        internal Task WriteByteSpan(ReadOnlySpan<byte> span, bool canAccumulate = true, TaskCompletionSource<object> completion = null)


Note: this is removed in the netcore/netfx specific files, and the netcore implementation is now in the shared TdsParserStateObject class

twsouthwick · 2025-02-13T16:25:44Z

@David-Engel these build failures appear to be occurring on main as well. Can I get a review here?

David-Engel · 2025-02-13T17:26:45Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs

@@ -8,6 +8,8 @@ namespace Microsoft.Data.SqlClient
 {
    internal partial class TdsParser
    {
+        private static readonly SqlObjectPool<ArrayBufferWriter<byte>> _writers = new(20, () => new(), a => a.Clear());


I dug into the SqlObjectPool logic because I was curious, so thought I would share in case anyone else was wondering. 20 here isn't a limit on parallelism or anything. It's just the maximum number that will be cached. Parallel Rents from the pool above 20 will simply get a new object. I agree that 20 should be plenty for the majority of scenarios.

twsouthwick · 2025-02-13T17:50:14Z

Looks like the "restore" step is failing for a bunch of the tests

David-Engel · 2025-02-13T17:58:30Z

Looks like the "restore" step is failing for a bunch of the tests

Yup. It's not your PR. It seems like a new underlying tooling bug on ARM64. We are investigating.

mdaigle · 2025-02-13T21:27:24Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs

+            }
+            try
+            {
+                TdsParser.ReliabilitySection.Assert("unreliable call to WriteByteArray");  // you need to setup for a thread abort somewhere before you call this method


@benrr101 FYI, you'll have a conflict on this line if you merge #3042 after this

mdaigle · 2025-02-13T22:00:18Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs

@@ -8,6 +8,8 @@ namespace Microsoft.Data.SqlClient
 {
    internal partial class TdsParser
    {
+        private static readonly SqlObjectPool<ArrayBufferWriter<byte>> _writers = new(20, () => new(), a => a.Clear());


Can you please add a comment in the code indicating the reasoning for 20?

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ArrayBufferWriter.cs

This was referenced Apr 8, 2024

Add hook for custom SSPI context for SqlConnection instances #2253

Open

Expose SqlAuthenticationParameters on SSPIContextProvider #2454

Draft

twsouthwick force-pushed the sspi-writer branch from e52f1c3 to 1ec663f Compare May 7, 2024 16:28

twsouthwick marked this pull request as ready for review May 7, 2024 21:20

David-Engel approved these changes May 13, 2024

View reviewed changes

David-Engel requested review from JRahnama, DavoudEshtehari and arellegue May 13, 2024 23:38

David-Engel added this to the 6.0-preview1 milestone Jun 3, 2024

twsouthwick added 4 commits June 27, 2024 11:15

Merge branch 'main' into sspi-writer

7c3335e

Merge remote-tracking branch 'upstream/main' into sspi-writer

4c3c3f2

fix

d3bd2b9

switch to span

266cf7f

David-Engel reviewed Jul 31, 2024

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SSPI/NegotiateSSPIContextProvider.cs Outdated Show resolved Hide resolved

add return

7d99053

twsouthwick mentioned this pull request Aug 19, 2024

Use string for SNI instead of byte[] #2790

Draft

twsouthwick added 3 commits August 19, 2024 11:17

Merge remote-tracking branch 'upstream/main' into sspi-writer

7e4d15f

revert

09db047

use return

a49abc3

inline

a1703d8

DavoudEshtehari reviewed Aug 21, 2024

View reviewed changes

revert other changes to string designer

b00a17c

twsouthwick marked this pull request as ready for review January 31, 2025 19:44

move write methods that are the same to shared file

43ec92d

twsouthwick commented Feb 1, 2025

View reviewed changes

twsouthwick force-pushed the sspi-writer branch from 74f7138 to c43f86f Compare February 3, 2025 18:59

make sure to use correct length

7b7ee36

twsouthwick force-pushed the sspi-writer branch from c43f86f to 7b7ee36 Compare February 3, 2025 21:38

Merge branch 'main' into sspi-writer

4f95969

paulmedynski requested review from a team and removed request for JRahnama and arellegue February 11, 2025 19:03

twsouthwick added 2 commits February 12, 2025 09:56

Merge remote-tracking branch 'origin/main' into sspi-writer

e1986ac

put method back

130cdef

David-Engel approved these changes Feb 13, 2025

View reviewed changes

mdaigle requested a review from a team February 13, 2025 17:32

mdaigle reviewed Feb 13, 2025

View reviewed changes

twsouthwick added 2 commits February 13, 2025 14:46

Add comment for pool

f40013e

Add note about file origin and editing restrictions

31e3ec9

mdaigle approved these changes Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use an IBufferWriter<byte> to write the outgoing SSPI blob #2452

Use an IBufferWriter<byte> to write the outgoing SSPI blob #2452

twsouthwick commented Apr 8, 2024 •

edited

Loading

twsouthwick commented May 7, 2024

codecov bot commented May 8, 2024 •

edited

Loading

twsouthwick commented May 13, 2024

David-Engel commented May 13, 2024

twsouthwick commented May 28, 2024

twsouthwick commented Aug 19, 2024

twsouthwick commented Aug 20, 2024

twsouthwick commented Aug 20, 2024

DavoudEshtehari Aug 21, 2024

twsouthwick Nov 15, 2024

David-Engel Feb 13, 2025

mdaigle Feb 13, 2025

twsouthwick commented Jan 31, 2025

twsouthwick commented Jan 31, 2025

David-Engel commented Jan 31, 2025

azure-pipelines bot commented Jan 31, 2025

twsouthwick commented Jan 31, 2025

David-Engel commented Jan 31, 2025

azure-pipelines bot commented Jan 31, 2025

twsouthwick commented Feb 1, 2025

twsouthwick Feb 1, 2025

twsouthwick commented Feb 13, 2025

David-Engel Feb 13, 2025

twsouthwick commented Feb 13, 2025

David-Engel commented Feb 13, 2025

mdaigle Feb 13, 2025

mdaigle Feb 13, 2025

Use an IBufferWriter<byte> to write the outgoing SSPI blob #2452

Are you sure you want to change the base?

Use an IBufferWriter<byte> to write the outgoing SSPI blob #2452

Conversation

twsouthwick commented Apr 8, 2024 • edited Loading

twsouthwick commented May 7, 2024

codecov bot commented May 8, 2024 • edited Loading

Codecov Report

twsouthwick commented May 13, 2024

David-Engel commented May 13, 2024

twsouthwick commented May 28, 2024

twsouthwick commented Aug 19, 2024

twsouthwick commented Aug 20, 2024

twsouthwick commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twsouthwick commented Jan 31, 2025

twsouthwick commented Jan 31, 2025

David-Engel commented Jan 31, 2025

azure-pipelines bot commented Jan 31, 2025

twsouthwick commented Jan 31, 2025

David-Engel commented Jan 31, 2025

azure-pipelines bot commented Jan 31, 2025

twsouthwick commented Feb 1, 2025

Choose a reason for hiding this comment

twsouthwick commented Feb 13, 2025

Choose a reason for hiding this comment

twsouthwick commented Feb 13, 2025

David-Engel commented Feb 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twsouthwick commented Apr 8, 2024 •

edited

Loading

codecov bot commented May 8, 2024 •

edited

Loading