Conversation
There was a problem hiding this comment.
Pull request overview
This PR sets the maxMessageSize property on various transport implementations and introduces error handling for messages that exceed size limits on the SmallWebRTCTransport. The changes ensure that each transport respects platform-specific message size constraints.
Changes:
- Sets
_maxMessageSizeto 1MB in WebSocketTransport (matching Python websockets default) - Adds message size validation and error handling to SmallWebRTCTransport with SCTP-based max message size
- Wraps Daily sendAppMessage in try-catch to handle message size errors
- Adds default case to OpenAIRealTimeWebRTCTransport sendMessage switch to throw UnsupportedFeatureError
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| transports/websocket-transport/src/webSocketTransport.ts | Sets max message size to 1MB to match Python websockets defaults |
| transports/small-webrtc-transport/src/smallWebRTCTransport.ts | Adds message size validation in sendMessage and sets maxMessageSize from SCTP config |
| transports/openai-realtime-webrtc-transport/src/OpenAIRealTimeWebRTCTransport.ts | Adds default case to sendMessage switch to properly handle unsupported message types |
| transports/daily/src/transport.ts | Wraps sendAppMessage in try-catch to convert Daily errors to MessageTooLargeError and sets maxMessageSize from internal property |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mattieruth
force-pushed
the
message-too-large-handling
branch
from
4a13a8a to
425f7e2
Compare
|
|
||
| dc.addEventListener("open", () => { | ||
| logger.debug("datachannel opened"); | ||
| this._maxMessageSize = this.pc?.sctp?.maxMessageSize ?? 64 * 1024; |
There was a problem hiding this comment.
I was inspecting this and noticed that Chrome requested (offer SDP) a max message size of:
a=max-message-size:262144
However, the answer (SDP) from aiortc was:
a=max-message-size:65536
So the default value we’re using is correct.
However, if we want to be 100% accurate, we could retrieve this exact value from the SDP answer, in case aiortc increases this limit in the future.
There was a problem hiding this comment.
per our discussion, this is happening with the above this.pc?.sctp?.maxMessageSize
There was a problem hiding this comment.
Yep, I have missed this one. 🙂
| - Added message size checking to `sendMessage()` to ensure no large | ||
| messages are sent, causing the data channel to fail and not recover. |
There was a problem hiding this comment.
Interesting, I haven’t tested this yet. Is this what happens if we try to send a message that’s too large, the data channel never recover ?
I would expect to just receive an error and for it to keep working afterward. 🤕
| ); | ||
| this._ws = null; | ||
| this._serializer = opts.serializer || new ProtobufFrameSerializer(); | ||
| this._maxMessageSize = 1024 * 1024; // python websockets default to 1MB |
There was a problem hiding this comment.
Would it make sense to allow users to change this default value when creating the transport ? I’m not sure whether they can change it on the server side if they have a custom WebSocket implementation.
There was a problem hiding this comment.
per our discussion, we think this is fine for now.
mattieruth
force-pushed
the
message-too-large-handling
branch
from
425f7e2 to
83d94ed
Compare
mattieruth
force-pushed
the
message-too-large-handling
branch
from
83d94ed to
b0990f7
Compare
introduced error handling for too-large messages on SmallWebRTCTransport
mattieruth
force-pushed
the
message-too-large-handling
branch
from
b0990f7 to
d434cc9
Compare
introduced error handling for too-large messages on SmallWebRTCTransport.
This PR depends on pipecat-ai/pipecat-client-web#168 being released.