Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP net: enable emitclose in socket #36662

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

WIP net: enable emitclose in socket #36662

wants to merge 1 commit into from

Conversation

dnlup
Copy link
Contributor

@dnlup dnlup commented Dec 28, 2020 •
edited
Loading

This is a WIP. It might take a while.

This PR should enable emitClose in net.Socket, ideally without changing any tests.

Fixes: #36636

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Dec 28, 2020
RaisinTen
RaisinTen reviewed Dec 28, 2020
View reviewed changes
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dnlup nit: I think it should be:
Ref: #36636
Fixes: #36636
@dnlup
Copy link
Contributor Author

dnlup commented Dec 28, 2020

@dnlup nit: I think it should be:
Ref: #36636
Fixes: #36636

Yup, thank you @RaisinTen
@dnlup dnlup force-pushed the feat/socket_close branch 5 times, most recently from 012e034 to afa462a Compare January 19, 2021 15:33
ronag
ronag reviewed Jan 20, 2021
View reviewed changes
lib/net.js Outdated Show resolved Hide resolved
ronag
ronag reviewed Jan 20, 2021
View reviewed changes
lib/net.js Show resolved Hide resolved
@dnlup
Copy link
Contributor Author

dnlup commented Jan 21, 2021

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 dnlup marked this pull request as ready for review January 21, 2021 09:44
dnlup
dnlup commented Jan 21, 2021
View reviewed changes
test/parallel/test-http2-respond-with-file-connection-abort.js
// 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.
Copy link
Contributor Author

@dnlup dnlup Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling destroy this way will cause HTTP2_NO_SOCKET_MANIPULATION, I don't see a way around that atm
dnlup
dnlup commented Jan 21, 2021
View reviewed changes
test/parallel/test-tls-socket-close.js
// 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 dnlup added the wip Issues and PRs that are still a work in progress. label Jan 23, 2021
@dnlup dnlup marked this pull request as draft February 12, 2021 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. wip Issues and PRs that are still a work in progress.
4 participants
@dnlup @ronag @RaisinTen @nodejs-github-bot