Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
mdaigle merged 25 commits intodotnet:mainfromtwsouthwick:sspi-writer
Feb 26, 2025

Conversation

twsouthwick
Copy link
Member

@twsouthwicktwsouthwick commentedApr 8, 2024•
edited
Loading

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
@twsouthwicktwsouthwick marked this pull request as ready for reviewMay 7, 2024 21:20
@twsouthwick
Copy link
MemberAuthor

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

@codecovCodecov
Copy link

codecovbot commentedMay 8, 2024•
edited
Loading

Codecov Report

Attention: Patch coverage is78.35821% with29 lines in your changes missing coverage. Please review.

Project coverage is 66.19%. Comparing base(0be2756) to head(e17288c).
Report is 4 commits behind head on main.

Files with missing linesPatch %Lines
.../src/Microsoft/Data/SqlClient/ArrayBufferWriter.cs56.25%21 Missing⚠️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs85.41%7 Missing⚠️
...t/Data/SqlClient/SSPI/NativeSSPIContextProvider.cs83.33%1 Missing⚠️

❗ There is a different number of reports uploaded between BASE (0be2756) and HEAD (e17288c). Click for more details.

HEAD has 1 upload less than BASE
FlagBASE (0be2756)HEAD (e17288c)
addons10
Additional details and impacted files
@@            Coverage Diff             @@##             main    #2452      +/-   ##==========================================- Coverage   72.81%   66.19%   -6.63%==========================================  Files         282      277       -5       Lines       59112    58829     -283     ==========================================- Hits        43045    38944    -4101- Misses      16067    19885    +3818
FlagCoverage Δ
addons?
netcore69.18% <89.15%> (-6.34%)⬇️
netfx64.87% <77.51%> (-6.28%)⬇️

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
Copy link
MemberAuthor

@David-Engel ping

@David-Engel
Copy link
Contributor

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

twsouthwick reacted with thumbs up emoji

@twsouthwick
Copy link
MemberAuthor

@David-EngelDavid-Engel added this to the6.0-preview1 milestoneJun 3, 2024
@twsouthwick
Copy link
MemberAuthor

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

David-Engel reacted with thumbs up emoji

@twsouthwick
Copy link
MemberAuthor

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

@twsouthwick
Copy link
MemberAuthor

@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?

@@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

twsouthwick reacted with thumbs up emoji
@azure-pipelinesAzure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@twsouthwick
Copy link
MemberAuthor

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

@@ -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)
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

@paulmedynskipaulmedynski requested review froma team and removed request forJRahnama andarellegueFebruary 11, 2025 19:03
@twsouthwick
Copy link
MemberAuthor

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

David-Engel reacted with thumbs up emoji

@@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

@mdaiglemdaigle requested a review froma teamFebruary 13, 2025 17:32
@twsouthwick
Copy link
MemberAuthor

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

@David-Engel
Copy link
Contributor

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.

twsouthwick reacted with thumbs up emoji

@@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

twsouthwick reacted with thumbs up emoji
@twsouthwick
Copy link
MemberAuthor

@mdaigle I've handled the merge conflicts

mdaigle reacted with thumbs up emoji

@mdaigle
Copy link
Contributor

/azp run

@azure-pipelinesAzure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@twsouthwick
Copy link
MemberAuthor

@mdaigle@David-Engel Is this good to merge in? Looks like main is still not running clean so I don't think my PR introduced the test failure I'm seeing

@mdaiglemdaigle added this to the7.0-preview1 milestoneFeb 26, 2025
@mdaigle
Copy link
Contributor

I kicked the failing job and expect it to pass shortly, then I'll merge 😄

twsouthwick reacted with hooray emoji

@mdaiglemdaigle merged commitdcf6ac4 intodotnet:mainFeb 26, 2025
123 checks passed
@twsouthwicktwsouthwick deleted the sspi-writer branchFebruary 26, 2025 19:09
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@DavoudEshtehariDavoudEshtehariDavoudEshtehari left review comments

@mdaiglemdaiglemdaigle approved these changes

@David-EngelDavid-EngelDavid-Engel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.1-preview1
Development

Successfully merging this pull request may close these issues.

5 participants
@twsouthwick@David-Engel@cheenamalhotra@mdaigle@DavoudEshtehari
[8]ページ先頭
©2009-2025 Movatter.jp