Skip to content

Commit

Permalink
Fix Envoy Mobile bug where writing prevents the read loop from running,
Browse files Browse the repository at this point in the history
by scheduling repeated onSendWindowAvailable calls in the next
dispatcher iteration.

Adds the regression test Charles cooked up in envoyproxy#2212

Fixes envoyproxy#2213

Signed-off-by: Ryan Hamilton <rch@google.com>
  • Loading branch information
RyanTheOptimist committed Apr 27, 2022
1 parent 7808a3d commit a943abc
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 2 deletions.
7 changes: 7 additions & 0 deletions library/common/event/provisional_dispatcher.cc
Expand Up @@ -44,6 +44,13 @@ envoy_status_t ProvisionalDispatcher::post(Event::PostCb callback) {
return ENVOY_SUCCESS;
}

Event::SchedulableCallbackPtr ProvisionalDispatcher::createSchedulableCallback(std::function<void()> cb) {
RELEASE_ASSERT(
isThreadSafe(),
"ProvisionalDispatcher::createSchedulableCallback must be called from a threadsafe context");
return event_dispatcher_->createSchedulableCallback(cb);
}

bool ProvisionalDispatcher::isThreadSafe() const {
// Doesn't require locking because if a thread has a stale view of drained_, then by definition
// this wasn't a threadsafe call.
Expand Down
9 changes: 9 additions & 0 deletions library/common/event/provisional_dispatcher.h
Expand Up @@ -41,6 +41,15 @@ class ProvisionalDispatcher : public ScopeTracker {
*/
virtual envoy_status_t post(Event::PostCb callback);

/**
* Allocates a schedulable callback. @see SchedulableCallback for docs on how to use the wrapped
* callback.
* @param cb supplies the callback to invoke when the SchedulableCallback is triggered on the
* event loop.
* Must be called from context where ProvisionalDispatcher::isThreadSafe() is true.
*/
virtual Event::SchedulableCallbackPtr createSchedulableCallback(std::function<void()> cb);

/**
* @return false before the Event::Dispatcher is running, otherwise the result of the
* underlying call to Event::Dispatcher::isThreadSafe().
Expand Down
5 changes: 3 additions & 2 deletions library/common/http/client.cc
Expand Up @@ -471,9 +471,10 @@ void Client::sendData(envoy_stream_t stream, envoy_data data, bool end_stream) {
if (direct_stream->explicit_flow_control_ && !end_stream) {
if (direct_stream->read_disable_count_ == 0) {
// If there is still buffer space after the write, notify the sender
// that send window is available.
// that send window is available, on the next dispatcher iteration.
direct_stream->wants_write_notification_ = false;
direct_stream->callbacks_->onSendWindowAvailable();
scheduled_callback_ = dispatcher_.createSchedulableCallback([direct_stream] {direct_stream->callbacks_->onSendWindowAvailable();});
scheduled_callback_->scheduleCallbackNextIteration();
} else {
// Otherwise, make sure the stack will send a notification when the
// buffers are drained.
Expand Down
1 change: 1 addition & 0 deletions library/common/http/client.h
Expand Up @@ -321,6 +321,7 @@ class Client : public Logger::Loggable<Logger::Id::http> {

ApiListener& api_listener_;
Event::ProvisionalDispatcher& dispatcher_;
Event::SchedulableCallbackPtr scheduled_callback_;
HttpClientStats stats_;
// The set of open streams, which can safely have request data sent on them
// or response data received.
Expand Down
19 changes: 19 additions & 0 deletions test/java/org/chromium/net/testing/BUILD
Expand Up @@ -90,3 +90,22 @@ envoy_mobile_android_test(
"//library/kotlin/io/envoyproxy/envoymobile:envoy_lib",
],
)

envoy_mobile_android_test(
name = "temporary_test",
srcs = [
"AndroidEnvoyExplicitH2FlowTest.java",
],
exec_properties = {
# TODO(lfpino): Remove this once the sandboxNetwork=off works for ipv4 localhost addresses.
"sandboxNetwork": "standard",
},
native_deps = [
"//library/common/jni:libndk_envoy_jni.so",
"//library/common/jni:libndk_envoy_jni.jnilib",
],
deps = [
":testing",
"//library/kotlin/io/envoyproxy/envoymobile:envoy_lib",
],
)

0 comments on commit a943abc

Please sign in to comment.