WIP net: enable emitclose in socket #36662
Draft
+34
−23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Yup, thank you @RaisinTen |
dnlup
force-pushed
the
feat/socket_close
branch
from
352ed68 to
38d61ae
Compare
dnlup
force-pushed
the
feat/socket_close
branch
5 times, most recently
from
012e034 to
afa462a
Compare
ronag
reviewed
Jan 20, 2021
ronag
reviewed
Jan 20, 2021
ronag
reviewed
Jan 20, 2021
ronag
reviewed
Jan 20, 2021
dnlup
force-pushed
the
feat/socket_close
branch
from
afa462a to
4db38c2
Compare
|
I am trying to figure out the test failures, maybe I should mark this draft as ready to run tests in the CI. This implementation is the one that makes more sense to me atm. |
dnlup
force-pushed
the
feat/socket_close
branch
from
4db38c2 to
175cf08
Compare
dnlup
commented
Jan 21, 2021
| // TODO: is this test needed? | ||
| // It errors with ERR_HTTP2_NO_SOCKET_MANIPULATION because we are | ||
| // calling destroy on the Proxy(ed) socket of the Http2ClientSession | ||
| // which causes `emit` to becalled and the error to be thrown. |
There was a problem hiding this comment.
Calling destroy this way will cause HTTP2_NO_SOCKET_MANIPULATION, I don't see a way around that atm
dnlup
commented
Jan 21, 2021
| // TODO: is this test needed? | ||
| // It fails with a timeout error because the `error` event is never emitted. | ||
| // `write` doesn't error, is that a good thing? | ||
|
|
There was a problem hiding this comment.
While trying different approaches in this PR I saw it is quite easy to cause SIGSEV errors in the tls module, so I might understand why this is here.
dnlup
force-pushed
the
feat/socket_close
branch
from
175cf08 to
8674991
Compare
dnlup
force-pushed
the
feat/socket_close
branch
from
8674991 to
b699090
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a WIP. It might take a while.
This PR should enable
emitCloseinnet.Socket, ideally without changing any tests.Fixes: #36636
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes