WIP | Prevent WriteAsync collision

[DavoudEshtehari]

Related issue #459 and #54
According to Microsoft documentation, WriteAsync a new method call just is able if there is not another one in progress.
This issue happened in a race condition, cause of asynchronous programming.

Update: Proposal extended and reworked in #796

[Trontin]

Any news on the progress of this PR ? It seems unactive since one month.

[DavoudEshtehari]

Any news on the progress of this PR ? It seems unactive since one month.

This PR is blocked by an issue in dotnet/runtime. You can follow it at #38185.

[roji]

Unless I'm mistaken, this is a synchronous call which blocks the current thread - that's a pretty bad idea in an async method (mixing sync and async can lead to all kinds of pain). Since _currentTask represents an asynchronous task (if I'm getting the code right), this is also a case of sync-over-async, which can cause deadlocks.

[cheenamalhotra]

It seems to fix the issue #459 (and some more related threads) and as we see in MS documentation for WriteAsync it says, "The SslStream class does not support multiple simultaneous write operations.", which I agree is against Async API design but I'm not sure if we have a better solution here. I guess that's why most of our APIs are sync-over-async but this mix and match of sync/async is one of the reasons behind colliding Async APIs.

Do you have a better solution in this case?

[roji]

The driver definitely needs to avoid starting a write if another one is in progress (the same is true for reads); this isn't just true for SslStream, but also for NetworkStream - and pretty much anything that does networking. FWIW I don't think this restriction is against async API design - async is about not blocking the thread, rather than about allowing multiple concurrent operations to occur on the same resource (i.e. connection).

More concretely... I don't doubt the importance of this fix (I have no context here) - all I'm saying is that any waiting for previous writes to complete should be done asynchronously, otherwise you're likely creating a new category of future bugs. One easy way to do this would be to have a SemaphoreSlim on the connection representing writing, and to simply call await WaitAsync on it. This would ensure only one writing operation occurs at any given moment, while making all waiting happens asynchronously. There may be some perf implication here, but I really don't have enough context to know how hot this code path is etc.

[roji]

BTW here's an example scenario on where sync-over-async could go wrong.

Say you have a lot of writers all of a sudden (e.g. because of a thundering herd effect or whatever). One of the writers wins and starts to do their thing; meanwhile, all the other enter the Wait call, blocking the thread. If your writers happen to be executed on the thread pool (which they are in ASP.NET, for example), then you've just exhausted the thread pool completely. The really bad part, is that when your very first writer completes - the one which actually started writing - it also needs a thread pool thread to execute the continuation (the code that runs after the async operation completed), but the pool is exhausted. This is a pseudo-deadlock, and your application will hang until the thread pool allocates new threads, which can take quite a while.

Since I'm lacking context, I don't know if the above can occur here specifically. But with all the bad scenarios around mixing sync and async code, it really best be avoided...

[DavoudEshtehari]

hi @roji
Thank you for the valuable point.

Since the sequence of calling the WriteAsync method is important in this scenario, and SemaphoreSlim doesn't respect the order, Do you have any solution/guidance for this case?

[roji]

@DavoudEshtehari apologies for disappearing for so long, I had some personal issues.

Looking at the current method as it is in the PR, I don't see any enforcing of ordering - if _currentTask is not-null (i.e. someone else is writing), we wait until that Task is over. I'm basically proposing substituting _currentTask for SemaphoreSlim as the synchronization check, that's all.

If you need to have multiple writers wait in line based on FIFO, then a more elaborate solution with ConcurrentQueue can be built - but I'd definitely need more context to understand needs here.

[DavoudEshtehari]

@roji thank you for your kind reply.

The issue might occur when more than one write request waits behind the semaphore to get permission to run the WriteAsync function. I think this is a possible condition.

Also, I've tried queue patterns but touching the WriteAsync throws the exception even though I wrap it up with a cold task.

[roji]

Note that with the current code, if more than one write request Waits on the Task, both will get unblocked the moment the task completes, and will concurrently call into base.WriteAsync - which is what this PR is trying to prevent. SemaphoreSlim would easily take care of that for you - when its WaitAsync method returns, it's guaranteed that only one request has acquired the semaphore. However, there is no guarantee of ordering - if more than one attempt did WaitAsync, they would get unblocked in "random" order.

[cheenamalhotra]

Hi @roji

Thanks for your tips, I got an implementation of Concurrent Queue with Semaphore Slim working (PR #796), would request you to take a look too.

However, this makes me think couldn't it be done by SslStream class itself? So that libraries don't land into surprises... It seems a basic case of ReadAsync/WriteAsync + parallel tasks is not supported by them and it can cause dangerous problems. Maybe a thread-safe design can be proposed to System.Net.Security?

[DavoudEshtehari]

It got fixed on PR #796.