search

fullstorydev / grpchan

Channels for gRPC: custom transports
MIT License
205 stars 23 forks source link

inprocgrpc: avoid racy goroutine leaks if/when context is cancelled #16

Closed jdef closed 6 years ago

jdef commented 6 years ago

See title.

jdef commented 6 years ago

CI linter is no longer golang 1.8 compat?

jhump commented 6 years ago

@jdef, indeed, two of the checks have had recent changes that make them 1.9 only. I will push a fix to master for this.

BTW, thanks for reporting this and submitting this PR! However, I don't think we want to increase the buffer size since that has ramifications for back-pressure/flow control. Instead, the RPCs should be using a select operation so they respect context cancellation (instead of being stuck trying to write to channel whose buffer is full).

I added a test to confirm the issue and have an alternate fix in #17.

jhump commented 6 years ago

Addressed with alternate fix in #17. Let me know if you still see anything screwy with this stuff, and thanks for bringing it to our attention!

jdef commented 6 years ago

I don't think we want to increase the buffer size since that has ramifications for back-pressure/flow control.

I don't think I follow your argument here. This is a single unary call, there's no backpressure to deal with because the chan receiver code is basically just setting options, or copying a data blob into a preallocated buffer, or bailing out.

The fix for #17 looks OK, and it's nice that you also wrote a test. Thanks for the fast response.

jhump commented 6 years ago

This is a single unary call, there's no backpressure to deal with

Oh yeah, good point!