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.

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

tls: use parent handle's close callback #2991

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Sep 21, 2015

When closing the child TLSWrap handle - wait for the proper parent's
handle close callback invocation. uv_close_cb may be invoked much
later than the next libuv tick, depending on the platform.

Fix: #2979

When closing the child TLSWrap handle - wait for the proper parent's
handle close callback invocation. `uv_close_cb` may be invoked much
later than the next libuv tick, depending on the platform.

Fix: nodejs#2979
@indutny
Copy link
Member Author

indutny commented Sep 21, 2015

cc @saghul

@indutny
Copy link
Member Author

indutny commented Sep 21, 2015

cc @nodejs/crypto @nodejs/collaborators

@brendanashworth brendanashworth added the tls Issues and PRs related to the tls subsystem. label Sep 21, 2015
@saghul
Copy link
Member

saghul commented Sep 22, 2015

@indutny #2979 (comment)

I'm not familiar with that code, but if it works as it says on the tin, it sounds good!

@jasnell
Copy link
Member

jasnell commented Sep 25, 2015

LGTM so long as CI is happy.

@indutny
Copy link
Member Author

indutny commented Sep 25, 2015

@jasnell thanks, let's wait for reporters to confirm that it fixes the problem for them.

@indutny
Copy link
Member Author

indutny commented Sep 25, 2015

@indutny
Copy link
Member Author

indutny commented Oct 12, 2015

Let's land this, just in case cc @nodejs/crypto . It is quite hard for XP people to get the build, and probably easier to just roll this out.

@bnoordhuis
Copy link
Member

Shouldn't this have some kind of regression test?

@indutny
Copy link
Member Author

indutny commented Oct 12, 2015

@bnoordhuis do you have any suggestions?

@bnoordhuis
Copy link
Member

Maybe a simple one that checks that the callback doesn't run before the 'close' event?

@indutny
Copy link
Member Author

indutny commented Oct 12, 2015

@bnoordhuis it doesn't on every platform except windows XP ;)

@bnoordhuis
Copy link
Member

Well, alright. LGTM. You should mention that in the commit log though.

@indutny
Copy link
Member Author

indutny commented Oct 12, 2015

Thank you, landing.

@indutny
Copy link
Member Author

indutny commented Oct 12, 2015

Landed in 51325c0, thank you!

@indutny indutny closed this Oct 12, 2015
@indutny indutny deleted the fix/gh-2979 branch October 12, 2015 17:29
indutny added a commit that referenced this pull request Oct 12, 2015
When closing the child TLSWrap handle - wait for the proper parent's
handle close callback invocation. `uv_close_cb` may be invoked much
later than the next libuv tick, depending on the platform.

The only platform that currently seem to defer `uv_close_cb` is Windows
XP. This behavior was not observed on other Windows systems, and is not
possible on Unixes.

Fix: #2979
PR-URL: #2991
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
indutny added a commit that referenced this pull request Oct 12, 2015
When closing the child TLSWrap handle - wait for the proper parent's
handle close callback invocation. `uv_close_cb` may be invoked much
later than the next libuv tick, depending on the platform.

The only platform that currently seem to defer `uv_close_cb` is Windows
XP. This behavior was not observed on other Windows systems, and is not
possible on Unixes.

Fix: #2979
PR-URL: #2991
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins
Copy link
Contributor

landed in lts-v4.x-staging as af10df6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
6 participants