Repository navigation
Conversation
A command written after the socket's finish event, but before its close event, could wait for drain forever. The peer had already ended the socket, so the write returned false and the socket was destroyed without emitting drain or error. Connection cleanup settled only pending reads, and socketTimeoutMS cannot fire on a destroyed socket. Connection cleanup now rejects a write that is waiting for drain with the connection error, and writeCommand checks the connection error before it writes, so a connection closed during serialization fails the command.
Contributor
This branch has not been deployed
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
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.
Description
Summary of Changes
A command written to a socket that is closing could wait for
drainforever. This change makes that write settle with the connection's error:Connection.cleanup()now rejects a write that is waiting fordrain, with the same error it already passes to a pending read.writeCommand()checks the connection's error after the command is serialized and before it writes. If the connection closed during serialization, the command fails instead of writing to a destroyed socket.netserver that ends every connection it accepts. Each test pins one of the two changes, and both fail onmainwithout this change.Notes for Reviewers
GitHub issues are disabled for this repository, and we could not open a NODE Jira ticket. This PR therefore includes the full reproduction below. Please associate a NODE ticket and add its scope to the title if required.
Why the write never settles (checked on Node.js v25.9.0):
allowHalfOpenfalse, Node ends the client socket and replacessocket.writewith an override for writes after FIN.closeis observed. Afterfinish, the socket is auto-destroyed in the same turn. Thecloseevent follows only in a later event-loop iteration, after the handle's close callback. In that windowconnection.closedis stillfalse, so the pool can still hand the connection out, andclient.close()can still sendendSessionsorabortTransactionon it.false. TheEPIPEgoes only to the write callback, which the driver does not pass, anddestroy(err)is a no-op on a destroyed socket. Neitherdrainnorerroris emitted, soonce(socket, 'drain')never settles.closefinally arrives,cleanup()settles onlydataEvents. The read loop has not started yet becausewriteCommand()is still awaitingdrain.socketTimeoutMScannot fire on a destroyed socket.timeoutForSocketWritebounds the wait only whentimeoutMSis set.rejectPendingWriteis a single field, likedataEvents, because a connection has at most one command in flight. Thefinallyblock clears it, andPromise.racealready handles a rejection that arrives afterdrainwon.No dependencies, package metadata, build scripts, generated files or vendored specification fixtures are changed.
What is the motivation for this change?
When a server closes pooled connections (for example, on shutdown or failover), a command sent in the window between the socket's
finishandcloseevents hangs forever. This includes the commandsMongoClient.close()sends, soclient.close()can hang too. The window is short. It is easy to hit on macOS, where kqueue can deliver the peer's FIN and the application's next step in the same event-loop turn, and it can be reproduced deterministically on any platform.For bug fixes
Current (incorrect) behavior:
A command written after the socket's
finishevent, but before itscloseevent, never resolves or rejects. Whenclosearrives, the connection is marked closed, but the command stays pending.MongoClient.close()hangs if it sends a command on such a connection.Expected behavior:
The command rejects with a
MongoNetworkErroronce the connection closes, andMongoClient.close()settles.How to reproduce:
Self-contained CommonJS script. It needs no MongoDB server, only a plain
netserver that ends every connection:Results:
mongodb@7.5.0andmongodb@7.7.0printstill pending after 5 seconds.rejected with MongoNetworkError: connection ... closed.The same window is reachable through the public API. In a test that triggers
client.close()from the pooled socket'sfinishevent while the server shuts down,client.close()stays pending until the process exits. With this change it settles.The new unit tests in
test/unit/cmap/connection.test.tscover both paths:finishevent but before itscloseevent was observed.Affected versions:
mongodb@7.5.0andmongodb@7.7.0, and onmainat 16136bb, with Node.js v25.9.0 on Linux.Double check the following
npm run check:lint): its stepsbuild:dts,check:dts,check:eslintandcheck:tsdpass, and the unit suite passestype(NODE-xxxx)[!]: description(no NODE ticket could be opened, see above)