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

test: handle SmartOS bug in test-tls-session-cache #7505

Closed
wants to merge 3 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 1, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test tls

Description of change

Sometimes, a SmartOS bug results in ECONNREFUSED when trying to connect
to the TLS server that the test starts. Retry in that situation.

Fixes: #5111
Refs: https://smartos.org/bugview/OS-2767

@Trott Trott added tls Issues and PRs related to the tls subsystem. test Issues and PRs related to the tests. smartos Issues and PRs related to the SmartOS platform. labels Jul 1, 2016
@Trott
Copy link
Member Author

Trott commented Jul 1, 2016

/cc @nodejs/testing

@Trott
Copy link
Member Author

Trott commented Jul 1, 2016

Stress test so-far showing (and hopefully will continue to show) that this version of the test is robust: https://ci.nodejs.org/job/node-stress-single-test/768/nodes=smartos14-64/console

Stress test showing the version on master branch is flaky: https://ci.nodejs.org/job/node-stress-single-test/767/nodes=smartos14-64/console

@Trott
Copy link
Member Author

Trott commented Jul 1, 2016

@Trott
Copy link
Member Author

Trott commented Jul 1, 2016

Stress test failed but now at a later part of the test, I think...

Sometimes, a SmartOS bug results in ECONNREFUSED when trying to connect
to the TLS server that the test starts. Retry in that situation.

Fixes: nodejs#5111
Refs: https://smartos.org/bugview/OS-2767
@Trott
Copy link
Member Author

Trott commented Jul 1, 2016

Rebased, force pushed, changed test to decrement a counter on failure, and trying stress test again:
https://ci.nodejs.org/job/node-stress-single-test/769/nodes=smartos14-64/console

@Trott
Copy link
Member Author

Trott commented Jul 2, 2016

That wasn't quite the right fix. Let's try again:

https://ci.nodejs.org/job/node-stress-single-test/775/nodes=smartos14-64/console

@Trott
Copy link
Member Author

Trott commented Jul 2, 2016

Hooray, that seems to have fixed it. PTAL @nodejs/testing

@santigimeno
Copy link
Member

LGTM. One (maybe evident) question though: how is that the process.exit is emitted twice?

@Trott
Copy link
Member Author

Trott commented Jul 2, 2016

@santigimeno exit is being emitted on process only once, but there are two listeners. The process.on('exit', ...) gets run twice because it's inside doTest() which is called twice. So, the listener gets added twice. It's behavior depends on testOptions which is scoped to doTest() so it will have different values (and therefore the listeners will behave differently) even though they are both being triggered by the same event.

@santigimeno
Copy link
Member

Oh. It was pretty obvious in hindsight. Thanks for the explanation @Trott

@Trott
Copy link
Member Author

Trott commented Jul 3, 2016

@Trott
Copy link
Member Author

Trott commented Jul 4, 2016

One build issue on CI but no issues with this test.

Trott added a commit to Trott/io.js that referenced this pull request Jul 4, 2016
Sometimes, a SmartOS bug results in ECONNREFUSED when trying to connect
to the TLS server that the test starts. Retry in that situation.

Fixes: nodejs#5111
Refs: https://smartos.org/bugview/OS-2767
PR-URL: nodejs#7505
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jul 4, 2016

Landed in fb4c022

@Trott Trott closed this Jul 4, 2016
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
Sometimes, a SmartOS bug results in ECONNREFUSED when trying to connect
to the TLS server that the test starts. Retry in that situation.

Fixes: #5111
Refs: https://smartos.org/bugview/OS-2767
PR-URL: #7505
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
@MylesBorins
Copy link
Contributor

@Trott lts?

@Trott
Copy link
Member Author

Trott commented Jul 12, 2016

@thealphanerd Yes if it lands cleanly.

@MylesBorins
Copy link
Contributor

does not land cleanly 😢

@Trott Trott deleted the opensslcli branch January 13, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
smartos Issues and PRs related to the SmartOS platform. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate flaky test-tls-session-cache
3 participants