- Notifications
You must be signed in to change notification settings - Fork1k
fix: Leaking yamux session after HTTP handler is closed#329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedFeb 18, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #329 +/- ##==========================================+ Coverage 63.61% 67.45% +3.84%========================================== Files 69 140 +71 Lines 786 7443 +6657 Branches 77 77 ==========================================+ Hits 500 5021 +4521- Misses 271 1910 +1639- Partials 15 512 +497
Continue to review full report at Codecov.
|
263f17a to6126814CompareCloses#317. The httptest server cancels the context after the connectionis closed, but if a connection takes a long time to close, the requestwould never end. This applies a context to the entire listener that cancelson test cleanup.After discussion with@bryphe-coder, reducing the parallel limit onWindows is likely to reduce failures as well.
| -name:Test with Mock Database | ||
| shell:bash | ||
| env: | ||
| GOCOUNT:${{ runner.os == 'Windows' && 3 || 5 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice addition 👍
| srv:=httptest.NewServer(handler) | ||
| srv:=httptest.NewUnstartedServer(handler) | ||
| srv.Config.BaseContext=func(_ net.Listener) context.Context { | ||
| ctx,cancelFunc:=context.WithCancel(context.Background()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice catch... It's surprising that this is necessary, but glad you found it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It genuinely amazed me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for working so hard to track these down@kylecarbs !
| -ubuntu-latest | ||
| -macos-latest | ||
| -windows-latest | ||
| -windows-2022 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Per our discussion in slack - we may want to consider bumping the cache in one of these ways:
- Adding a
-v0-parameter that we can bump to invalidate the cache - Using
matrix.osinstead ofrunner.osfor the cache here - that will usewindows-2022instead ofwindowsfor the cache key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ahh that's a great idea! Changing now.
2a0a153 to6c68e41Compare
Closes#317. The httptest server cancels the context after the connection
is closed, but if a connection takes a long time to close, the request
would never end. This applies a context to the entire listener that cancels
on test cleanup.
After discussion with@bryphe-coder, reducing the parallel limit on
Windows is likely to reduce failures as well.