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

Prevent Concurrent Writes #2356

Closed
wants to merge 1 commit into from
Closed

Conversation

steve-gray
Copy link

When writing via multiple goroutines, it's possible to end up causing the grpc-go streams to deadlock/block if you don't guard against this by putting a mutex outside calls to SendMsg. This is fine, until you want to use a compressor/decompressor - and suddenly these operations now mean you are blocking sends while compression happens - and there's no way to mitigate this externally, short of writing your own compression system on top of your gRPC base protocol.

Fixes #2355

When writing via multiple goroutines, it's possible to end up causing the grpc-go streams to deadlock/block if you don't guard against this by putting a mutex *outside* calls to SendMsg. This is fine, until you want to use a compressor/decompressor - and suddenly these operations now mean you are blocking sends while compression happens - and there's no way to mitigate this externally, short of writing your own compression system on top of your gRPC base protocol.
@thelinuxfoundation
Copy link

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
CLA GitHub bot

@steve-gray
Copy link
Author

@thelinuxfoundation CLA signed.

@dfawley
Copy link
Member

dfawley commented Oct 19, 2018

@steve-gray thanks for the contribution. Unfortunately, we do not want to enable concurrent SendMsg calls at this time. There are a few reasons I can provide for this:

  1. Because ordering often does matter for streams, it could be confusing for users if we allowed this.
  2. Streams not attempting this type of operation become penalized by the cost of taking this mutex.
  3. Similarly, in a heavily loaded system, attempting to send in parallel will not end up leading to any performance gains; the CPU and network are already fully saturated, and the cost of synchronization becomes a negative.
  4. If we were to allow this, the issue of fairness arises pretty quickly thereafter. sync.Mutexes have no concept of fairness, so it's possible that even if the user implements their own message reordering on the receiving end, one message could be postponed until many subsequent messages (hundreds/thousands/more) have been sent. This could result in problems for users not expecting that possibility - e.g. if they are buffering a message until all prior messages are received.

If this is a performance bottleneck for your use case, I would recommend something like what @snowzach described in #1879. Thanks again for the PR.

@dfawley dfawley closed this Oct 19, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Apr 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Concurrent Calls to SendMsg can hang
3 participants