proxy: close yamux session properly#94
Conversation
When cleaning up, we need to make sure the yamux session is closed as well. It does not close the session by just closing the listener. OTOH, it closes all streams if the session is closed. Fixes: kata-containers#93 Signed-off-by: Peng Tao <bergwolf@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #94 +/- ##
==========================================
+ Coverage 34.4% 37.74% +3.34%
==========================================
Files 2 2
Lines 250 257 +7
==========================================
+ Hits 86 97 +11
+ Misses 151 149 -2
+ Partials 13 11 -2 |
|
lgtm |
| defer func() { | ||
| close(closeCh) | ||
| lp.Close() | ||
| s.Close() |
There was a problem hiding this comment.
The test is closing in the opposite order to the order in proxy.go.
|
This looks good but doesn't resolve the issue with #89: taking just the top commit from that PR and applying on top of this still causes a test race. Note that you need to run the tests a few times to actually hit the race though. |
|
@jodh-intel yeah, the PR is not complete. It turns out that yamux stream is closed in an asynchronous manner. I'll see how to fix it. |
|
@jodh-intel Now the data race should be closed. I've run 20 times (merged with #89 for sure) without hitting it. PTAL. |
|
Build failed (third-party-check pipeline) integration testing with
|
sboeuf
left a comment
sboeuf
left a comment
There was a problem hiding this comment.
Globally looks good, but I think the handling of the WaitGroup is racy.
| } | ||
|
|
||
| go proxyConn(conn, stream) | ||
| go proxyConn(conn, stream, wg) |
There was a problem hiding this comment.
This is potentially racy. Usually, we call wg.Add() before to call into the go routine calling wg.Done().
The current go routine here might reach wg.Done() before the subsequent go routines will call into wg.Add().
bergwolf
force-pushed
the
session_close
branch
2 times, most recently
from
82e3301 to
2df1fe5
Compare
yamux stream closing is aync. Use a wait group to make sure when we quit the serve() goroutine, all the stream copy goroutines have quited as well. Signed-off-by: Peng Tao <bergwolf@gmail.com>
|
Build failed (third-party-check pipeline) integration testing with
|
|
Merge it since the failed three checks are all expected.
|
what's the story on this ?
|
|
@sboeuf About openstack-zuul, yes, |
|
ok thanks for the feedback |
4 participants
When cleaning up, we need to make sure the yamux session is closed as
well. It does not close the session by just closing the listener.
OTOH, it closes all streams if the session is closed.
Fixes: #93