bug: serialize send to allow for concurrent session use #132
No reviewers
Labels
No labels
breaking-change
bug
dependencies
duplicate
enhancement
go
good first issue
help wanted
invalid
investigation
proposal
proposal-accepted
question
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
nemith/netconf!132
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "brb/push-ywyqwrvmxrpp"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
While doing some performance testing it was clear that I ditched the previous serialization for writes. The old v0.0.2 method was just to put all sends behind a mutex. This is a bit more advanced using an outbound queue and writing each message as they come in.
Pull request overview
This PR introduces serialized message sending to enable concurrent session use. Previously, the code allowed concurrent writes which could cause issues with the transport layer that only supports one active writer at a time. The new implementation uses an outbound message queue processed by a dedicated goroutine to serialize all writes.
Key changes:
sendLoop()goroutine andoutQchannel to serialize all outbound messagespendingReqtopendingRespfor better semantic clarityDo()method to usequeueSend()instead of writing directly to transportReviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
sendLoop(),queueSend(), andsendMsg()functions; addsoutQchannel to Session struct; refactors Do() to use queue-based sending; starts sendLoop goroutine after handshakependingResptype instead ofpendingReq💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The outQ channel is created as an unbuffered channel. This means that queueSend will block at line 352 until sendLoop is ready to receive from it. If sendLoop is slow or blocked, this could cause all callers of queueSend (including Do) to block. Consider using a buffered channel to allow some queueing without blocking the sender, or document this blocking behavior clearly.
@ -338,1 +433,4 @@if !errors.Is(err, io.EOF) {s.logger.Error("failed to receive message", "error", err)}}If the outQ channel is closed (which happens during session Close at line 663), sending to it at line 352 will panic. This can occur if Do is called concurrently with Close. The function should check if the session is closing or use a recover mechanism, or the Close method should prevent new messages from being queued before closing the channel.
The new concurrent send functionality introduced by sendLoop and queueSend lacks adequate test coverage. Specifically, there are no tests that: 1) verify that calling Do concurrently with Close doesn't cause panics or deadlocks, 2) ensure that sendLoop properly drains the queue before exiting, or 3) validate that the session handles the case where messages are queued while the session is closing. Consider adding tests for these edge cases to prevent regressions.
There's a critical race condition in the Close method. After closing the outQ channel at line 663, any concurrent goroutine that attempts to call Do (which calls queueSend) will panic when trying to send on the closed channel at line 352 in queueSend. Additionally, there's no synchronization to wait for sendLoop to finish processing queued messages before closing the transport at line 666. This could result in sendLoop attempting to write to a closed transport. Consider: 1) checking the closing flag before attempting to queue messages, 2) using a sync.WaitGroup to wait for sendLoop to exit after closing outQ, and 3) ensuring proper error handling for operations attempted after Close is called.
it blocks on receive. This is fine.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@ -586,1 +681,3 @@}type closeSession struct {XMLName xml.Name `xml:"close-session"`}The Close method is not protected against being called multiple times. If Close is called twice, the second call will panic when trying to close the already-closed channels at lines 687 and 690. Consider using sync.Once or checking if the channels are already closed before attempting to close them again.
The Transport struct documentation should be updated to reflect the new restriction that only one active reader and one active writer are allowed at a time. This is a significant behavioral constraint that callers need to be aware of.
@ -67,0 +79,4 @@if t.activeWriter {return nil, transport.ErrStreamBusy}The error message io.ErrClosedPipe is misleading in this context. The pipe is not closed; rather, there is already an active reader. Consider using a more descriptive custom error such as errors.New("reader already active") to make debugging easier for users of this test utility.
The error message io.ErrClosedPipe is misleading in this context. The pipe is not closed; rather, there is already an active writer. Consider using a more descriptive custom error such as errors.New("writer already active") to make debugging easier for users of this test utility.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.