Perf improvement on queue/table bindings in high throughput scenarios (V1) #1657
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fabiocav
requested changes
Apr 19, 2018
|
|
||
| private readonly CloudStorageAccount _storageAccount; | ||
| private readonly RandomNameResolver _resolver; | ||
| private readonly JobHostConfiguration _hostConfiguration; | ||
|
|
||
| public AsyncCancellationEndToEndTests() | ||
| { | ||
| // Run tests in multithreaded environment | ||
| _oldContext = SynchronizationContext.Current; | ||
| SynchronizationContext.SetSynchronizationContext(null); |
There was a problem hiding this comment.
Can you wrap this in a try/finally block to ensure the synchronization context is restored?
There was a problem hiding this comment.
Sorry, I misread the diff here. But after chatting, I do think it would be a better approach and (safer in the long run) if we were to scope this per method, ensuring the context is restored at the end of the method call.
mhoeger
force-pushed
the
queueBindingFix
branch
from
d609fc8 to
cd83a34
Compare
fabiocav
approved these changes
Apr 19, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Dupe of PR #1631, which had strange merge behavior
In the binding flow, there is a point where we transition from async code => sync code => async code. In the sync => async transition, we force the async code to run synchronously by using Task.Run(() => AsyncMethod().GetAwaiter().GetResult() on the async method.
This pattern leads to thread pool contention.
Task.Run() creates a child thread, AsyncMethod() does the work that the parent thread needs, and GetAwaiter().GetResult() blocks the parent thread until the work from AsyncMethod() is completed. In low throughput scenarios, this pattern doesn't cause visible issues because there are enough threads in the thread pool to complete the work in AsyncMethod(). However, when there is a high volume of requests through this code path that blocks on GetAwaiter().GetResult(), the child threads that are spun up to do work that unblock the parents end up queued, waiting for blocked parent threads (this happens because there are a limited number of threads in the thread pool, read [2] for more details). Although work eventually completes, significant delays have led to timeouts (this fix therefore resolves #1467).
Background
There are a number of places where we are not writing our methods so that they are sync OR async all the way down (should not be a combination of both). This blog post [1] from Stephen Toub details some of the dangers of wrapping a synchronous method in an async wrapper. This blog post [2] details some of the dangers with wrapping an async method in an sync wrapper
[1] Async over sync
[2] Sync over async