envoyproxy / envoyproxy/envoy-mobile

core: improve handling of race between local stream cancellation and non-terminal encoding

Open
#647 0 comments 0 reactions 0 assignees View on GitHub
core no stalebot
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.