fix: ensure udp messages are sent on close #946
Conversation
WalkthroughThe pull request modifies the core socket functionality by updating the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client/Caller
participant S as SocketTcp (EventEmitter)
C->>S: send(message)
S-->>S: Increment _pendingMessages
alt send successful
S-->>S: Decrement _pendingMessages
else send error
S-->>S: onError called\nDecrement _pendingMessages
end
alt _pendingMessages equals 0
S->>S: Emit 'idle' event
end
C->>S: close()
alt _pendingMessages > 0
S->>S: Wait for 'idle' event
end
S-->>C: Socket closed
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/socket.ts (1)
194-209: Remove thereturnstatement to align with thevoidreturn type.
Static analysis warns that returning a value from a function declared asvoidcan be misleading. Since the returned value is never consumed, you can safely remove it:- return this.socket.send(data, this.port, this.hostname, (err) => { + this.socket.send(data, this.port, this.hostname, (err) => {🧰 Tools
🪛 Biome (1.9.4)
[error] 195-209: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/socket.ts(6 hunks)test/index.spec.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/socket.ts
[error] 195-209: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
🔇 Additional comments (4)
test/index.spec.ts (1)
401-422: Consider awaiting theclient.connect()call for clarity and potential race avoidance.
By not awaiting theconnect()call, there may be sporadic race conditions if the test attempts to verifypendingMessagesbefore the client finishes connection procedures. If this is intentional (e.g., verifying message queuing during connection), then it’s fine; otherwise, consider:- client.connect(); + await client.connect();Do you want me to open a follow-up PR with this adjustment?
src/socket.ts (3)
6-7: Nice extension of theSocketclass fromEventEmitter.
This improvement allows emitting and listening to custom events (e.g.,'idle') more flexibly.Also applies to: 19-19
132-132: Good introduction of_pendingMessagestracking and theidleaccessor.
This approach neatly encapsulates the queue tracking logic and clarifies when the socket is free.Also applies to: 153-153, 173-178
212-221: Well-implemented asyncclosemethod.
Waiting for the'idle'event ensures that no messages are lost, aligning nicely with reliability requirements.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/tcp.spec.ts (1)
116-134: Excellent test case for the new message waiting functionality!This test effectively verifies that:
- The socket correctly tracks pending messages (counter increments to 4)
- The idle state reflects when messages are being processed
- The close method properly waits for all messages to complete
- The 'idle' event is correctly emitted once
One minor improvement opportunity:
Consider adding a test assertion to verify that sending a message after close doesn't increment the pending messages counter (line 130). While you're implicitly testing this with the next assertion, an explicit check would make the test more comprehensive:
await socket.close(); socket.send('something'); + t.is(socket.pendingMessages, 0, 'Sending after close should not increment pending messages'); t.is(socket.pendingMessages, 0); t.is(socket.idle, true); t.true(emit.calledOnceWith('idle'));src/socket.ts (2)
116-131: Message tracking implementation looks good, with a minor defensive coding opportunity.The implementation properly tracks pending messages and emits the 'idle' event when appropriate.
Consider adding a defensive check to ensure the counter doesn't go negative, which could happen in edge cases:
this.socket.write(data + '\n', (err) => { if (this._pendingMessages) { this._pendingMessages -= 1; } + // Ensure counter never goes below zero as a safeguard + if (this._pendingMessages < 0) { + this._pendingMessages = 0; + } if (err) { try { this.onError(err);
229-236: Good async implementation of UDP close.The implementation correctly waits for pending messages to complete before closing the socket.
For consistency with the TCP implementation, consider moving the
this.connected = falseline inside the promise callback:await new Promise((res) => { this.socket.close(res as () => void); - this.connected = false; + this.connected = false; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/socket.ts(4 hunks)test/index.spec.ts(1 hunks)test/tcp.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/index.spec.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/socket.ts
[error] 212-226: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
🔇 Additional comments (7)
test/tcp.spec.ts (1)
112-114: Good addition of pending message checks!These assertions validate that the socket correctly reports its state after a send attempt on a closed connection.
src/socket.ts (6)
6-7: Good choice extending EventEmitter!Using EventEmitter provides a clean way to signal when all pending messages have been processed.
13-13: Well-designed tracking of pending messages.Adding this protected property allows both TCP and UDP implementations to track messages consistently.
20-20: Required super() call for EventEmitter.Correctly initializes the EventEmitter parent class.
28-28: Proper initialization of the counter.Good practice to initialize the counter in the constructor.
40-45: Clean accessor implementation for state inspection.These getters provide a nice, consistent interface for checking the message state without exposing the internal counter directly.
134-138: Good implementation of async close with waiting.Making the close method async and using the 'once' utility to wait for the 'idle' event is an elegant solution.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
🎉 This PR is included in version 5.1.1-next.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.1.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🚨 Proposed changes
[[Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.]]
⚙️ Types of changes
What types of changes does your code introduce? Put an
xin the boxes that applySummary by CodeRabbit
New Features
Tests
SocketTcpduring the closing process, ensuring proper management of message queue and idle state.