envoyproxy / envoyproxy/envoy-mobile
core: improve handling of race between local stream cancellation and non-terminal encoding
- Dominant language
- Java
- Stars
- 566
- Forks
- 85
- PR merge metrics
- No merged PRs in 30d
Description
https://github.com/lyft/envoy-mobile/pull/616 moved stream cancellation (and its atomic state) from the platform layer to the core layer, here. This change was made to solidify two platform-level implementations into one implementation in the core layer. Moreover, it allowed Envoy Mobile to have test coverage where it didn't before.
However, it introduced a subtle race between Dispatcher::resetStream's onCancel and any of encodeHeaders/Data's callbacks that are _not_ terminal. The race happens because the two callbacks are being enqueued onto the same dispatch queue/ran on the same executor by two different threading contexts _after_ the atomic check of the closed_ state happens. This means that they could be serialized in either order; whereas we want to guarantee that _no_ callback will be executed after onCancel fires in the application. The lock protects the critical region between the call to dispatchable, and after the call that dispatches the appropriate callback. There should not be much lock contention because most calls will happen from the single-threaded context of the Envoy Main thread (encodeHeaders/Data).
Contributor guide
Assessment
This issue has not been assessed yet.