Outbound flow control bugfix #61

marcoferrer · 2019-08-04T21:43:18Z

No description provided.

This reverts commit 11d7169

marcoferrer · 2019-08-04T21:46:52Z

...st/kotlin/com/github/marcoferrer/krotoplus/coroutines/server/ServerCallBidiStreamingTests.kt

-                        requestIter.next(),
-                        requestIter.next(),
+                        requestIter.next().also { requestIter.hasNext() },
+                        requestIter.next().also { requestIter.hasNext() },


This was required after updating to coroutines 1.3.0-RC. Every call to next() must be preceded by a call to hasNext()

marcoferrer · 2019-08-04T21:48:01Z

...st/kotlin/com/github/marcoferrer/krotoplus/coroutines/server/ServerCallBidiStreamingTests.kt

+                    .consumeAsFlow()
+                    .collect {
+                        responseChannel.send(HelloReply.newBuilder().setMessage("Reply: ${it.name}").build())
+                    }


The mapTo operator on channel is now deprecated. With out this refactor, the test would fail.

marcoferrer · 2019-08-04T21:49:12Z

.../com/github/marcoferrer/krotoplus/coroutines/integration/ClientStreamingBackPressureTests.kt

@@ -74,10 +75,10 @@ class ClientStreamingBackPressureTests {

    @Test
    fun `Client send suspends until server invokes receive`() {
-        lateinit var serverRequestChannel: ReceiveChannel<HelloRequest>


This change was to prevent race conditions between the serverRequestChannel var being populated and test assertions running.

marcoferrer · 2019-08-04T21:50:33Z

...st/kotlin/com/github/marcoferrer/krotoplus/coroutines/client/ClientCallBidiStreamingTests.kt

@@ -332,4 +343,58 @@ class ClientCallBidiStreamingTests {
        assert(responseChannel.isClosedForReceive) { "Response channel should be closed for receive" }
    }

+    @Test
+    fun `High throughput call succeeds`() {


This test was introduced as part of the discussion the was had with @chris-blacker in #59

marcoferrer · 2019-08-04T21:50:51Z

...st/kotlin/com/github/marcoferrer/krotoplus/coroutines/client/ClientCallBidiStreamingTests.kt

@@ -149,7 +160,7 @@ class ClientCallBidiStreamingTests {
                requestChannel.close()
            }

-            responseChannel.map { it.message }.toList()
+            responseChannel.consumeAsFlow().map { it.message }.toList()


Map operator was deprecated

marcoferrer · 2019-08-04T21:51:27Z

...st/kotlin/com/github/marcoferrer/krotoplus/coroutines/client/ClientCallBidiStreamingTests.kt

@@ -55,6 +62,10 @@ class ClientCallBidiStreamingTests {
    @[Rule JvmField]
    var grpcServerRule = GrpcServerRule().directExecutor()

+    @[Rule JvmField]
+    public val timeout = CoroutinesTimeout.seconds(30)


This allows us to debug coroutine deadlocks in the unit tests.

marcoferrer · 2019-08-04T21:54:54Z

...s-coroutines/src/main/kotlin/com/github/marcoferrer/krotoplus/coroutines/call/FlowControl.kt

@@ -42,40 +43,51 @@ internal fun <T> CallStreamObserver<*>.applyInboundFlowControl(
    }
 }

+private typealias MessageHandler = suspend ActorScope<*>.() -> Unit
+
 internal fun <T> CoroutineScope.applyOutboundFlowControl(


The outbound flow control handler has been refactored. It no longer spawns multiple jobs when applying backpressure and can properly handle superfluous invocations of the on ready handler runnable.

marcoferrer · 2019-08-05T02:25:22Z

Looks like the failing test in CI could be unrelated to the code change. Its possible it has something to do with the dependency version bump.

com.github.[secure].krotoplus.coroutines.server.ServerCallServerStreamingTests > Server responds with error when scope cancelled exceptionally FAILED
    java.lang.IllegalStateException
        Caused by: java.lang.reflect.InvocationTargetException
            Caused by: java.lang.UnsatisfiedLinkError

Tests seem consistently stable locally. @chris-blacker Could you verify your use case against this branch?
I'll work on sorting out the UnsatisfiedLinkError in the mean time.

blachris · 2019-08-05T11:20:11Z

...s-coroutines/src/main/kotlin/com/github/marcoferrer/krotoplus/coroutines/call/FlowControl.kt

+    }
+
+    val messageHandlerActor = actor<MessageHandler>(
+        capacity = Channel.UNLIMITED,


A limited capacity didn't work? I am wondering if it's possible that the channel becomes a memory leak if jobs are added faster than the worker consumes them.

Thats a good catch. I must've changed it while debugging. I'll test it with the value reverted to CONFLATED.

This implementation is based off the native grpc util StreamObservers.copyWithFlowControl() which ensured each invocation of the onReadyHandler always ran. source

I am using CONFLATED and have no issues.

blachris

Good solution. I tested the new flow control in my setup and the issue has been fixed.

blachris · 2019-08-11T14:43:20Z

...s-coroutines/src/main/kotlin/com/github/marcoferrer/krotoplus/coroutines/call/FlowControl.kt

+        capacity = Channel.UNLIMITED,
+        context = Dispatchers.Unconfined + CoroutineExceptionHandler { _, e ->
+            streamObserver.completeSafely(e)
+            targetChannel.close(e)


I think it is missing that the targetChannel is canceled in some situations. I tried canceling it here but this code was not being executed when I expected.
I have encountered that a client bidi call send hangs when the call is getting canceled in another thread because the channel is not canceled. This happened when the call is canceled while a send was pending in the call rendezvous outboundChannel. I think the rpc scope then somehow cleans up without canceling the channel, thus the send hangs.

blachris · 2019-08-11T14:47:24Z

...s-coroutines/src/main/kotlin/com/github/marcoferrer/krotoplus/coroutines/call/FlowControl.kt

+        for(handler in channel){
+            if(isCompleted.get()) break
+            handler(this)
+        }


Adding this here reduced the problem but didn't eliminate it. I guess there must be other paths for the scope to cancel.

catch(ex: CancellationException) { targetChannel.cancel(ex) throw ex }

So the scope could cancellation can also be propagated from its parent under normal normal coroutine usage. This case was covered before because executing new launch on a cancelled scope would take care. Its hard to reproduce but Im trying a few things now.

marcoferrer · 2019-08-11T18:22:05Z

In regards to the UnsatisfiedLinkError, it looks like the source of the issue is related to using the kotlinx-coroutines-debug artifact coupled with mockk. Details are outlined here. Ill temporarily comment out the dependency.

marcoferrer · 2019-08-12T03:21:25Z

...rc/test/kotlin/com/github/marcoferrer/krotoplus/coroutines/integration/BidiStreamingTests.kt

+    }
+
+    @Test
+    fun `Client cancellation cancels server rpc scope`() {


@chris-blacker I think I was able to reproduce your hanging client issue with this test. It turns out that the outbound message handler wasnt bound to normal channel cancellation. Thats been fixed in the latest changes. Since the actor wasnt bound, client coroutines would hang since one of their children would never complete.

codecov-io · 2019-08-12T03:24:10Z

Codecov Report

Merging #61 into master will increase coverage by 0.42%.
The diff coverage is 85.71%.

@@             Coverage Diff              @@
##             master      #61      +/-   ##
============================================
+ Coverage     85.66%   86.09%   +0.42%     
  Complexity       19       19              
============================================
  Files            15       15              
  Lines           279      302      +23     
  Branches         42       48       +6     
============================================
+ Hits            239      260      +21     
- Misses           13       15       +2     
  Partials         27       27

Impacted Files	Coverage Δ	Complexity Δ
...oferrer/krotoplus/coroutines/server/ServerCalls.kt	`81.7% <100%> (-0.8%)`	`0 <0> (ø)`
...otoplus/coroutines/client/ClientBidiCallChannel.kt	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
...s/coroutines/client/ClientResponseStreamChannel.kt	`85.71% <50%> (-14.29%)`	`9 <1> (ø)`
...rcoferrer/krotoplus/coroutines/call/FlowControl.kt	`83.33% <84%> (+8.33%)`	`0 <0> (ø)`	⬇️
...utines/call/FlowControlledInboundStreamObserver.kt	`79.16% <0%> (+4.16%)`	`0% <0%> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a643a4...4375f58. Read the comment docs.

blachris · 2019-08-13T08:48:21Z

Thanks for the changes. I think they go in the right direction but the hang-on-bidi-call-close issue is not fully resolved for me. I will investigate and give you more information and/or a failing test.

marcoferrer · 2019-08-13T13:21:59Z

Using the coroutines debug probes might shed some light as to what is stuck in suspension in your use case

blachris · 2019-08-21T12:48:52Z

I tried but I cannot not reproduce the hang-on-bidi-call-close issue outside of one specific stress scenario. And in that scenario it only happens during the shutdown and I can work around it. You can see my attempt here but the test passes fine.
I would say that the issue may be related to my application and this branch of Kroto+ works fine. How do you feel about advancing the branch to a release version eventually?

marcoferrer · 2019-08-21T12:55:39Z

That’s good to hear. I can work on getting this merged and into a release today. There’s a few spots that need a little clean up first.

marcoferrer added 7 commits August 4, 2019 17:30

update dependencies

125e6a3

fix typo in config message name (#57)

757da7f

clean up usages of deprecated apis

11d7169

fix race condition in unit back-pressure test

bed8d00

add test from @chris-blacker for high volume race condition

fec2f7d

Revert "clean up usages of deprecated apis"

5df774a

This reverts commit 11d7169

fix race condition in outbound flow control handler

6b7fdea

marcoferrer commented Aug 4, 2019

View reviewed changes

marcoferrer added 4 commits August 4, 2019 20:46

update outbound flow control

7621579

add coroutine timeout rule to unit tests

afd315e

debug ci issues

42a9460

downgrade grpc version

4da8aaf

blachris reviewed Aug 5, 2019

View reviewed changes

blachris previously approved these changes Aug 5, 2019

View reviewed changes

blachris mentioned this pull request Aug 5, 2019

Bugfix for race condition in outbound flow control #59

Closed

blachris reviewed Aug 11, 2019

View reviewed changes

propagate scope cancellation to target channel in outbound flow control

d99af22

marcoferrer dismissed blachris’s stale review via d99af22 August 11, 2019 16:32

marcoferrer added 3 commits August 11, 2019 23:05

configure detailed test output

bdba585

disable coroutines debug artifact

11b91e3

update dependency versions

18aac4f

marcoferrer added 4 commits August 11, 2019 23:09

add bidi streaming integration tests

8e9bd7c

propagate client inbound channel close as call cancellation

b4bd070

close outbound message handler in server call handlers

59715c5

improve determinism of tests

5f326a3

marcoferrer commented Aug 12, 2019

View reviewed changes

marcoferrer added 4 commits August 11, 2019 23:35

Update build.gradle

4a667e7

Update CallOptionsTest.kt

7760fcd

add withContext to atomic server bidi streaming test

c76b00b

fix flaky cancellation message checking

19c0ce4

blachris previously approved these changes Aug 21, 2019

View reviewed changes

marcoferrer dismissed blachris’s stale review via 70c20ce August 22, 2019 01:57

marcoferrer force-pushed the outbound-flow-control-bugfix branch 2 times, most recently from fbb77bd to 9bfb977 Compare August 22, 2019 02:24

update tests

9bcd734

marcoferrer force-pushed the outbound-flow-control-bugfix branch from 9bfb977 to 9bcd734 Compare August 22, 2019 02:34

update tests

4375f58

marcoferrer merged commit 0454f13 into master Aug 22, 2019

marcoferrer deleted the outbound-flow-control-bugfix branch August 23, 2019 00:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Outbound flow control bugfix #61

Outbound flow control bugfix #61

marcoferrer commented Aug 4, 2019

marcoferrer Aug 4, 2019

marcoferrer Aug 4, 2019

marcoferrer Aug 4, 2019

marcoferrer Aug 4, 2019

marcoferrer Aug 4, 2019

marcoferrer Aug 4, 2019

marcoferrer Aug 4, 2019

marcoferrer commented Aug 5, 2019

blachris Aug 5, 2019 •

edited

marcoferrer Aug 5, 2019

blachris Aug 21, 2019

blachris left a comment

blachris Aug 11, 2019

blachris Aug 11, 2019

marcoferrer Aug 11, 2019

marcoferrer commented Aug 11, 2019

marcoferrer Aug 12, 2019

codecov-io commented Aug 12, 2019 •

edited

blachris commented Aug 13, 2019

marcoferrer commented Aug 13, 2019 •

edited

blachris commented Aug 21, 2019

marcoferrer commented Aug 21, 2019

Outbound flow control bugfix #61

Outbound flow control bugfix #61

Conversation

marcoferrer commented Aug 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoferrer commented Aug 5, 2019

blachris Aug 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blachris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoferrer commented Aug 11, 2019

Choose a reason for hiding this comment

codecov-io commented Aug 12, 2019 • edited

Codecov Report

blachris commented Aug 13, 2019

marcoferrer commented Aug 13, 2019 • edited

blachris commented Aug 21, 2019

marcoferrer commented Aug 21, 2019

blachris Aug 5, 2019 •

edited

codecov-io commented Aug 12, 2019 •

edited

marcoferrer commented Aug 13, 2019 •

edited