test: https server close event by braydonf · Pull Request #5106 · nodejs/node · GitHub

Closes: #5083

The commit should target the test: subsystem rather than net:.

Nit: Instead of using the serverCloseEventCalled boolean, use common.mustCall() on the close callback.

const here please

LGTM if CI is green. Few nits tho.

https://ci.nodejs.org/job/node-test-pull-request/1581/

@jasnell This is a test that needs a bugfix. It is intended to fail. It should not land until there is a bug fix (assuming this is a bug and not expected behavior being misconstrued as a bug, but it looks like a genuine bug to me).

Technically, I guess this shouldn't be a pull request and should instead be put in #5083. But the folks reporting this have done such a great job tracking down everything relevant to their bug report that I'm not inclined to complain.

Anyway, with that context, I guess we can look at the all-red CI as a win. (Well, except the lint warning which says that assert is defined but not used.)

For a bug fix, restoring the server property on socket fixes this particular test. This can be done by removing lines (381 and 382) in TLSSocket.prototype._init of _tls_wrap.js:

 if (socket && socket.server === this.server)
    socket.server = null;

However these two tests will not pass:

test/parallel/test-tls-dhe.js
test/parallel/test-tls-ticket-cluster.js

Also, yep the goal with opening as a PR is to have CI run and having the failing tests (so we can fix). We can put a WIP on this if that's necessary.

Bug fix found, all tests passing locally.

CI: https://ci.nodejs.org/job/node-test-pull-request/1598/

CI looks good. (Just one known flaky test that should be fixed when #5154 lands.)

LGTM but sign-off from someone who has spent some quality time with _tls_wrap.js would probably be good. Maybe one or more of @bnoordhuis @indutny @shigeki @mscdex ?

Sorry, just found this after proposing #5262 . Please let me know if you still want to pursue this PR's approach.

@braydonf 7885b1d has been landed. Is this PR still relevant?

I ran the test included in this PR with the fix in #5262 and everything passed, perhaps we should include this test also.

@braydonf sounds like a good idea, may I ask you to strip all other changes from this PR then?

Okay rebased with only the test included.

Made a backup of the branch with other changes, for reference.

CI: https://ci.nodejs.org/job/node-test-pull-request/1688/

CI is green except one unrelated failure.

LGTM

Landing.

LGTM

Landed in 210e65a, thank you!

PR-URL: #5106 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>

This test is failing in v4.x-staging 😭
@nodejs/testing thoughts?

@thealphanerd I think this test depends on 7885b1d so if that didn't land on v4.x-staging, that might explain it.

7885b1d just landed and this is now landing cleanly. Thank you as always for your swift and accurate response

PR-URL: #5106 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>

braydonf mentioned this pull request Feb 5, 2016

socket.server is not defined #5083

Closed

mscdex added https Issues or PRs related to the https subsystem. test Issues and PRs related to the tests. labels Feb 5, 2016

braydonf force-pushed the server-close branch from d1d7854 to 094f507 Compare February 5, 2016 23:51

braydonf changed the title net: add test for https server close event test: add test for https server close event Feb 5, 2016

Trott reviewed Feb 6, 2016
View reviewed changes

braydonf force-pushed the server-close branch from 094f507 to f183608 Compare February 6, 2016 02:54

jasnell added the lts-watch-v4.x label Feb 7, 2016

jasnell reviewed Feb 7, 2016
View reviewed changes

braydonf force-pushed the server-close branch from a124228 to 2d3b2c0 Compare February 9, 2016 05:36

braydonf changed the title test: add test for https server close event https: fix https server close event Feb 9, 2016

braydonf force-pushed the server-close branch from 2d3b2c0 to 88e8dd4 Compare February 9, 2016 05:53

mscdex added tls Issues and PRs related to the tls subsystem. net Issues and PRs related to the net subsystem. and removed https Issues or PRs related to the https subsystem. labels Feb 9, 2016

test: add test for https server close event

630eb1a

braydonf force-pushed the server-close branch from 5f289e3 to 630eb1a Compare February 17, 2016 19:48

braydonf changed the title https: fix https server close event test: https server close event Feb 17, 2016

indutny closed this Feb 19, 2016

indutny pushed a commit that referenced this pull request Feb 19, 2016

test: add test for https server close event

210e65a

PR-URL: #5106 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>

rvagg pushed a commit that referenced this pull request Feb 21, 2016

test: add test for https server close event

c55bb79

PR-URL: #5106 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>

MylesBorins pushed a commit that referenced this pull request Mar 1, 2016

test: add test for https server close event

9ee459b

PR-URL: #5106 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 1, 2016

MylesBorins pushed a commit that referenced this pull request Mar 1, 2016

test: add test for https server close event

7705360

PR-URL: #5106 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>

MylesBorins pushed a commit that referenced this pull request Mar 2, 2016

test: add test for https server close event

b935aa2

PR-URL: #5106 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>

test: https server close event by braydonf · Pull Request #5106 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: https server close event - #5106

test: https server close event#5106braydonf wants to merge 1 commit intonodejs:masternodejs/node:masterfrom braydonf:server-closebraydonf/node:server-closeCopy head branch name to clipboard

Conversation

braydonf commented Feb 5, 2016

Uh oh!

mscdex commented Feb 5, 2016

Uh oh!

Trott Feb 6, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell Feb 7, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Feb 7, 2016

Uh oh!

jasnell commented Feb 7, 2016

Uh oh!

Trott commented Feb 7, 2016

Uh oh!

braydonf commented Feb 8, 2016

Uh oh!

braydonf commented Feb 8, 2016

Uh oh!

braydonf commented Feb 9, 2016

Uh oh!

Trott commented Feb 9, 2016

Uh oh!

Trott commented Feb 9, 2016

Uh oh!

indutny commented Feb 16, 2016

Uh oh!

indutny commented Feb 17, 2016

Uh oh!

braydonf commented Feb 17, 2016

Uh oh!

indutny commented Feb 17, 2016

Uh oh!

braydonf commented Feb 17, 2016

Uh oh!

indutny commented Feb 17, 2016

Uh oh!

indutny commented Feb 17, 2016

Uh oh!

jasnell commented Feb 19, 2016

Uh oh!

indutny commented Feb 19, 2016

Uh oh!

indutny commented Feb 19, 2016

Uh oh!

indutny commented Feb 19, 2016

Uh oh!

MylesBorins commented Mar 1, 2016

Uh oh!

Trott commented Mar 1, 2016

Uh oh!

MylesBorins commented Mar 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

test: https server close event#5106
braydonf wants to merge 1 commit into
nodejs:masterfrom
braydonf:server-close