Skip to content

bug: serialize send to allow for concurrent session use#132

Merged
nemith merged 1 commit into
mainfrom
brb/push-ywyqwrvmxrpp
Jan 8, 2026
Merged

bug: serialize send to allow for concurrent session use#132
nemith merged 1 commit into
mainfrom
brb/push-ywyqwrvmxrpp

Conversation

@nemith

@nemith nemith commented Jan 7, 2026

Copy link
Copy Markdown
Owner

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • Added sendLoop() goroutine and outQ channel to serialize all outbound messages
  • Renamed pendingReq to pendingResp for better semantic clarity
  • Refactored Do() method to use queueSend() instead of writing directly to transport

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
session.go Implements outbound message queue with sendLoop(), queueSend(), and sendMsg() functions; adds outQ channel to Session struct; refactors Do() to use queue-based sending; starts sendLoop goroutine after handshake
session_test.go Updates test to use renamed pendingResp type instead of pendingReq

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread session.go
Comment thread session.go
Comment thread session.go Outdated
Comment thread session.go
@nemith nemith force-pushed the brb/push-ywyqwrvmxrpp branch 2 times, most recently from 3f8636d to 8d85dce Compare January 8, 2026 16:31
@nemith nemith requested a review from Copilot January 8, 2026 16:32
@nemith nemith force-pushed the brb/push-ywyqwrvmxrpp branch 2 times, most recently from de5ff0d to e4c67d7 Compare January 8, 2026 16:36

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread session.go Outdated
Comment thread testutil/transport.go
Comment thread testutil/transport.go
Comment thread testutil/transport.go

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nemith nemith merged commit c280df0 into main Jan 8, 2026
12 checks passed
@nemith nemith deleted the brb/push-ywyqwrvmxrpp branch January 8, 2026 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants