search

marcoferrer / kroto-plus

gRPC Kotlin Coroutines, Protobuf DSL, Scripting for Protoc
Apache License 2.0
494 stars 28 forks source link

Outbound flow control bugfix #61

Closed marcoferrer closed 5 years ago

marcoferrer commented 5 years ago

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.

marcoferrer commented 5 years ago

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.

codecov-io commented 5 years ago

Codecov Report

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

Impacted file tree graph

@@             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> (ø) :arrow_down:
...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> (ø) :arrow_down:
...utines/call/FlowControlledInboundStreamObserver.kt 79.16% <0%> (+4.16%) 0% <0%> (ø) :arrow_down:

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 commented 5 years ago

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 commented 5 years ago

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

blachris commented 5 years ago

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 commented 5 years ago

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.