Fix Envoy Mobile bug where writing prevents the read loop from running, by RyanTheOptimist · Pull Request #2221 · envoyproxy/envoy-mobile

RyanTheOptimist · 2022-04-27T22:38:52Z

Fix Envoy Mobile bug where writing prevents the read loop from running,
by scheduling repeated onSendWindowAvailable calls in the next
dispatcher iteration.

Adds the regression test Charles cooked up in #2212

Fixes #2213

Risk Level: Low
Testing: New regression test
Docs Changes: N/A
Release Notes: N/A

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>

RyanTheOptimist · 2022-04-27T22:40:06Z

@alyssawilk please take a look. I'm not entirely sure if this is the right approach.

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist · 2022-04-27T23:45:24Z

FYI: This PR is not complete; I'm looking for guidance if this approach seems reasonable.

alyssawilk

yeah I think this is the right away to go about it, thanks for tackling it! I'd be inclined to unit test as well as you get it ready.

alyssawilk · 2022-04-28T13:58:32Z

library/common/http/client.cc

      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.


Think it's worth commenting why the delay?

Sure. Done.

alyssawilk · 2022-04-28T14:00:28Z

library/common/http/client.cc

+        // that send window is available, on the next dispatcher iteration.
        direct_stream->wants_write_notification_ = false;
-        direct_stream->callbacks_->onSendWindowAvailable();
+        scheduled_callback_ = dispatcher_.createSchedulableCallback(


can't remember offhand: do we need to create a new one each time, or only the one time?

Oh nice. That seems to work. Done!

apparently it didn't work - can we call out why we always need to create in a comment?

Good point. Done.

Signed-off-by: Ryan Hamilton <rch@google.com>

alyssawilk

LBTM :-)

RyanTheOptimist

Unit test coverage added via the changes to the existing tests.

RyanTheOptimist · 2022-04-28T17:28:35Z

library/common/http/client.cc

      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.


Sure. Done.

RyanTheOptimist · 2022-04-28T17:28:46Z

library/common/http/client.cc

+        // that send window is available, on the next dispatcher iteration.
        direct_stream->wants_write_notification_ = false;
-        direct_stream->callbacks_->onSendWindowAvailable();
+        scheduled_callback_ = dispatcher_.createSchedulableCallback(


Oh nice. That seems to work. Done!

RyanTheOptimist · 2022-04-28T18:01:26Z

@danzh2010 FYI

RyanTheOptimist · 2022-04-28T18:02:23Z

@jpsim Would you mind taking a look at this? Or redirect if someone else would be a better choice?

RyanTheOptimist · 2022-04-28T18:37:20Z

@goaway as per our discussion in the community meeting, what do you think of this change?

goaway

Based on our discussion this past week, and given the fact that this is all effectively guarded by the explicit flow control flag (which is off by default), I think it's reasonable to move forward with this.

Thanks for the discussion and investigation @RyanTheOptimist and @carloseltuerto.

goaway · 2022-05-05T19:57:34Z

Rerunning the failed CI job.

Signed-off-by: Ryan Hamilton <rch@google.com>

alyssawilk

ugh, I want to prematurely optimize that callback creation so badly :-P

goaway · 2022-05-10T18:51:15Z

Thanks, @RyanTheOptimist!

carloseltuerto · 2022-05-10T19:11:47Z

When removing this line,

@Ignore("https://github.com/envoyproxy/envoy-mobile/issues/2213")

This unfortunately, this ends up with crash more than 50% of the time:

#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x0000000105cb1b00, pid=77193, tid=32123
#
# JRE version: OpenJDK Runtime Environment Zulu11.50+19-CA (11.0.12+7) (build 11.0.12+7-LTS)
# Java VM: OpenJDK 64-Bit Server VM Zulu11.50+19-CA (11.0.12+7-LTS, mixed mode, tiered, compressed oops, g1 gc, bsd-aarch64)
# Problematic frame:
# May 10, 2022 7:06:33 PM io.netty.handler.logging.LoggingHandler channelInactive
INFO: [id: 0xc19fa4fa, L:/0:0:0:0:0:0:0:0:61521] INACTIVE
May 10, 2022 7:06:33 PM io.netty.handler.logging.LoggingHandler channelUnregistered
INFO: [id: 0xc19fa4fa, L:/0:0:0:0:0:0:0:0:61521] UNREGISTERED
V  [libjvm.dylib+0x379b00]  jni_GetObjectClass+0x120
#
# No core dump will be written. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again
#

* main: build: remove dist/ (#2184) Fix Envoy Mobile bug where writing prevents the read loop from running, (#2221) Add comments to CronetBidirectionalStream (#2266) CronetBidirectionalStream (#2164) ci: update build image (#2261) Bump Lyft Support Rotation (#2260) Signed-off-by: JP Simard <jp@jpsim.com>

* main: Add assert when failing to get_env (#2253) Update Kotlin standard libraries to 1.6.21 (#2256) iOS: Change release artifacts to use xcframeworks (#2217) build: remove dist/ (#2184) Fix Envoy Mobile bug where writing prevents the read loop from running, (#2221) Add comments to CronetBidirectionalStream (#2266) CronetBidirectionalStream (#2164) Signed-off-by: JP Simard <jp@jpsim.com>

Fix Envoy Mobile bug where writing prevents the read loop from running,

a943abc

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>

RyanTheOptimist force-pushed the AsyncOnSendWindowAvailable branch from 9902a88 to a943abc Compare April 27, 2022 22:39

RyanTheOptimist requested a review from alyssawilk April 27, 2022 22:39

RyanTheOptimist added 2 commits April 27, 2022 22:45

format

63ac2b7

Signed-off-by: Ryan Hamilton <rch@google.com>

Add missing test file

bc5ba5a

Signed-off-by: Ryan Hamilton <rch@google.com>

alyssawilk reviewed Apr 28, 2022

View reviewed changes

RyanTheOptimist added 3 commits April 28, 2022 17:13

Add unit test coverage

9070956

Signed-off-by: Ryan Hamilton <rch@google.com>

Reuse scheduled_callback_

b8c62e0

Signed-off-by: Ryan Hamilton <rch@google.com>

comment

08b8adf

Signed-off-by: Ryan Hamilton <rch@google.com>

alyssawilk previously approved these changes Apr 28, 2022

View reviewed changes

RyanTheOptimist commented Apr 28, 2022

View reviewed changes

RyanTheOptimist assigned jpsim Apr 28, 2022

RyanTheOptimist mentioned this pull request May 2, 2022

onResponseHeaders not called on continuous sendData #2213

Closed

goaway previously approved these changes May 5, 2022

View reviewed changes

RyanTheOptimist dismissed stale reviews from goaway and alyssawilk via ad82119 May 6, 2022 19:14

Always schedule a new callback to capture new state

648522a

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist force-pushed the AsyncOnSendWindowAvailable branch from ad82119 to 648522a Compare May 7, 2022 14:45

RyanTheOptimist added 2 commits May 9, 2022 16:55

Merge branch 'main' into AsyncOnSendWindowAvailable

123ce21

Add comment

c2107cc

Signed-off-by: Ryan Hamilton <rch@google.com>

alyssawilk approved these changes May 9, 2022

View reviewed changes

goaway approved these changes May 10, 2022

View reviewed changes

goaway merged commit 643831b into envoyproxy:main May 10, 2022

Conversation

RyanTheOptimist commented Apr 27, 2022

Uh oh!

RyanTheOptimist commented Apr 27, 2022

Uh oh!

RyanTheOptimist commented Apr 27, 2022

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist commented Apr 28, 2022

Uh oh!

RyanTheOptimist commented Apr 28, 2022

Uh oh!

RyanTheOptimist commented Apr 28, 2022

Uh oh!

goaway left a comment

Choose a reason for hiding this comment

Uh oh!

goaway commented May 5, 2022

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

goaway commented May 10, 2022

Uh oh!

carloseltuerto commented May 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants