fix(ext/node): use node:timers for http server connection checks, expo#33483
Merged
bartlomieju merged 2 commits intoApr 26, 2026
Merged
Conversation
bartlomieju
reviewed
Apr 25, 2026
|
|
||
| const interval = this.connectionsCheckingInterval || 30_000; | ||
| this[kConnectionsCheckingInterval] = core.createSystemInterval( | ||
| this[kConnectionsCheckingInterval] = setInterval( |
Member
There was a problem hiding this comment.
This will cause sanitizer issues in tests. The previous code was correct
bartlomieju
requested changes
Apr 25, 2026
fibibot
approved these changes
Apr 25, 2026
fibibot
left a comment
fibibot
left a comment
Contributor
There was a problem hiding this comment.
LGTM. The follow-up commit (b8a62128, "fix sanitizer leak: wrap createSystemInterval handle for _destroyed") replaces the bare core.createSystemInterval ID with a ConnectionsCheckingInterval wrapper class ({ _timerId, _destroyed }), which both addresses the sanitizer leak @bartlomieju raised on the prior round and gives the upstream test the _destroyed property it asserts on:
setupConnectionsTracking()→destroyConnectionsCheckingInterval(old)→core.createSystemInterval(...)→ store new handle. The destroy helper no-ops whenhandle._destroyed, so a re-listen path doesn't double-cancel.httpServerPreClose(server)calls the same destroy helper, so closing the server cancels the underlying timer and flips_destroyed = true. The previous code zeroedserver[kConnectionsCheckingInterval]after cancelling; the new code leaves the (now-destroyed) handle in place because the upstream test readsserver[kConnectionsCheckingInterval]._destroyedafter close.kConnectionsCheckingIntervalis now exported from_http_server, matchingrequire('_http_server').kConnectionsCheckingIntervalin the test.
Confirmed against denoland/node_test parallel/test-http-server-clear-timer.js: it emits listening twice and asserts _destroyed on the prior interval and on the saved interval after server.close(). The wiring in this PR satisfies both halves.
CI rerunning, 0 failures so far.
Enables tests/node_compat/runner/suite/test/parallel/test-http-server-clear-timer.js Co-authored-by: Divy Srivastava <[email protected]>
Reviewer flagged that switching to setInterval triggered the test sanitizer (interval started in test but never completed). Revert to core.createSystemInterval / core.cancelTimer and instead wrap the system-interval id in a small ConnectionsCheckingInterval handle that carries the Node-compatible _destroyed flag the test reads off server[kConnectionsCheckingInterval]. Co-authored-by: Divy Srivastava <[email protected]>
divybot
force-pushed
the
claude/test-http-server-clear-timer
branch
from
April 26, 2026 02:37
800f412 to
fd7a45c
Compare
bartlomieju
approved these changes
Apr 26, 2026
1 task
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
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Enables
test-http-server-clear-timerin node_compat suite.Test plan
cargo test --test node_compat -- test-http-server-clear-timer