FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: deflake http-server-request-timeout test by nicksia-vgw · Pull Request #44169 · nodejs/node · GitHub

/ node Public

test: deflake http-server-request-timeout test - #44169

Closed
nicksia-vgw wants to merge 1 commit into
nodejs:mainfrom
nicksia-vgw:fix-flaky-http-server-request-timeouts-test
Closed

test: deflake http-server-request-timeout test#44169
nicksia-vgw wants to merge 1 commit into
nodejs:mainfrom
nicksia-vgw:fix-flaky-http-server-request-timeouts-test

Conversation

Copy link
Copy Markdown
Contributor

parallel/http-server-request-timeouts-mixed test was sometimes failing due to insufficient tolerance between the connection timeout checking interval, and the expected timeout specified in the test.

The checking interval was 500ms, and the request was checked for timeout exactly 500ms after the request was expected to timeout. This led to a timing condition where the next check would occur slightly after the request was expected to timeout.

Detailed Explanation
Timings
headersTimeout: 2000ms
requestTimeout: 4000ms
connectionsCheckingInterval: 500ms

Request 2 is started at headersTimeout * 0.2, or 400ms.
Headers time out 2000ms after that.
Completion of Request 2 is checked at at headersTimeout * 1.2 + connectionsCheckingInterval, or 2900ms.

Timeline
@[400ms]: Request 2 client created and first write sent.
@[2400ms]: Headers are timed out.
@[2900ms]: Request 2 completion is checked (which is intended to depend on Request 2 expiring via headersTimeout, as seen in the comment - assert(request2.response.startsWith(responseTimeout)); // It is expired due to headersTimeout.

The problem is that the connectionsCheckingInterval is slightly too high. The connection checking interval could tick just after 2400ms, and then just after 2900ms.

Example

@[2424ms] request2.completed: false
checking connection timeout
@[2525ms] request2.completed: false
@[2625ms] request2.completed: false
@[2727ms] request2.completed: false
@[2828ms] request2.completed: false
node:assert:400
    throw err;
    ^

AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

  assert(request2.completed)

    at Timeout._onTimeout (/Users/nicksia/Documents/git/node/test/parallel/test-http-server-request-timeouts-mixed.js:108:5)
    at listOnTimeout (node:internal/timers:564:17)
    at process.processTimers (node:internal/timers:507:7) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}

The fix is to ensure connection checking interval is less than 500ms, and to decouple the check timeout from connection checking interval.

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Aug 8, 2022
nicksia-vgw force-pushed the fix-flaky-http-server-request-timeouts-test branch 2 times, most recently from 723159d to 86c0d69 Compare August 8, 2022 10:14
parallel/http-server-request-timeouts-mixed test was sometimes failing
due to insufficient tolerance between the connection timeout checking
interval, and the expected timeout specified in the test.

This change makes the checking interval more frequent, and decouples
the timeout for the check from the checking interval.

fixes: nodejs#43465
nicksia-vgw force-pushed the fix-flaky-http-server-request-timeouts-test branch from 86c0d69 to 3c68bf1 Compare August 8, 2022 10:15
lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 8, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 8, 2022

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

lpinca commented Aug 10, 2022
edited
Loading

Copy link
Copy Markdown
Member

Superseds #43470.

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Contributor Author

Could I get a hand getting this merged please?

MoLow commented Aug 14, 2022

Copy link
Copy Markdown
Member

@nicksia-vgw I see this has still failed on osx: https://ci.nodejs.org/job/node-test-commit-osx/46808/
are we ok with that?

Copy link
Copy Markdown
Contributor Author

Oh sorry - I missed that. Let me take a look.

Copy link
Copy Markdown

I think for timing sensitive tests like these, it might be more reliable when run as part of the sequential suite, to avoid issues caused by delays due to resource contention.

Any concerns with merging this PR (it will reduce failure rate), then moving these failing tests to the sequential suite in a follow-up PR?

  • parallel_test_http_server_request_timeouts_mixed
  • parallel_test_http_server_headers_timeout_keepalive

itsnicksia commented Aug 14, 2022
edited
Loading

Copy link
Copy Markdown

OK - I'm ready to merge.

lpinca commented Aug 15, 2022

Copy link
Copy Markdown
Member

Landed in b84a633.

lpinca pushed a commit that referenced this pull request Aug 15, 2022
parallel/test-http-server-request-timeouts-mixed.js test was sometimes
failing due to insufficient tolerance between the connection timeout
checking interval, and the expected timeout specified in the test.

This change makes the checking interval more frequent, and decouples
the timeout for the check from the checking interval.

PR-URL: #44169
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
lpinca closed this Aug 15, 2022
danielleadams pushed a commit that referenced this pull request Aug 16, 2022
parallel/test-http-server-request-timeouts-mixed.js test was sometimes
failing due to insufficient tolerance between the connection timeout
checking interval, and the expected timeout specified in the test.

This change makes the checking interval more frequent, and decouples
the timeout for the check from the checking interval.

PR-URL: #44169
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
ruyadorno pushed a commit that referenced this pull request Aug 23, 2022
parallel/test-http-server-request-timeouts-mixed.js test was sometimes
failing due to insufficient tolerance between the connection timeout
checking interval, and the expected timeout specified in the test.

This change makes the checking interval more frequent, and decouples
the timeout for the check from the checking interval.

PR-URL: #44169
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Copy link
Copy Markdown
Member

This depends on #42893, which is marked as "dont-land-on-v16.x".

This file contains hidden or 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL