Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implements SslStreamProxy write synchronization with SemaphoreSlim #1

Open
wants to merge 1 commit into
base: WriteAsyncCollision
Choose a base branch
from

Conversation

JAZDaddy
Copy link

Following feedback on dotnet#579 (dotnet#579 (comment)), implemented synchronization using SemaphoreSlim to avoid race conditions (and the expected recurrence of the original SslStream WriteAsync failure) on completion of _currentTask.Wait. We've been able to scale up the TVP size on tests above 1MM rows without issue and get past this critical, release-blocking error in our Docker-based .NET Core solution with this implementation.

…synchronization using SemaphoreSlim to avoid race conditions on completion of _currentTask.Wait.
@DavoudEshtehari
Copy link
Owner

Hi @JAZDaddy

Thank you for the suggested code and for sharing the investigation results.
We already have an in-progress change. The main concern is the order of requests that might be changed in a load condition. We have a plan to back to it for further investigation.
Please, don't stop sharing your thoughts.

@JAZDaddy
Copy link
Author

@DavoudEshtehari Thanks for your feedback; I'd be interested to hear the direction this is going in order to deal with the ordering concern. We're likely to hold onto this SemaphoreSlim for our own use in the interim since we're dead in the water without it.

I apologize in advance if what follows is incredibly simplistic...

Coming to this without the context you're immersed in, it's hard to get a handle on whether the SslStream's (sensible) single-writer demand is the core issue (thus synchronizing in SslStreamProxy as you have) versus having simultaneous callers requesting writes in the first place. It seems like the async paradigm flies in the face of the underlying assumptions of single-threaded synchrony in TDS. It's almost like each writer really wants to have the stream to itself until its batch is complete instead of viewing the stream as a simple common resource. If that's the case, it seems that the message, rather than the packet, is the more accurate locking granularity. Again, apologies if that's not even close...

@DavoudEshtehari
Copy link
Owner

Hi @JAZDaddy

FYI: Proposal extended and reworked in #796
Feel free to review/test it.

winseros pushed a commit to winseros/SqlClient that referenced this pull request Jan 17, 2021
DavoudEshtehari pushed a commit that referenced this pull request Feb 22, 2021
* Removing BinaryFormatter from NetFx

* review comments

* fix version typo

* remove extra line

* Reverted SqlException Test

* review comments

* Review comment

* Desrialize

* addressing review comments

* Fix exception in deserialization (#1)

* review comments

* add extra line to the end of strings designer

* end of line

Co-authored-by: jJRahnama <jrahnama@simba.com>
Co-authored-by: Karina Zhou <v-jizho2@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants