Make WebSockets Timeout #448
Conversation
|
This pull request has been linked to Shortcut Story #57052: Wrap the WebSocket stream in a |
sburmanoctopus
force-pushed
the
sast/websocket-timeouts
branch
from
48525a3
to
93e712b
Compare
…eploy/Halibut into sast/websocket-timeouts
|
|
||
| namespace Halibut.Transport.Streams | ||
| { | ||
| class CallUnderlyingAsyncMethodsStream : AsyncStream |
There was a problem hiding this comment.
To ensure we covered all the methods that NetworkTimeoutStream would call, we copied that class, and altered appropriately.
sburmanoctopus
changed the title
| @@ -20,6 +20,7 @@ public class TcpKeepAliveTests : BaseTest | |||
| { | |||
| [Test] | |||
| [LatestClientAndLatestServiceTestCases(testNetworkConditions: false, testListening:false, testWebSocket:false)] | |||
| // TCP Keep alives were for TcpClients, not web sockets | |||
There was a problem hiding this comment.
Interesting find - side project: Do keep alives work as expected for WebSockets or did we miss something?
There was a problem hiding this comment.
Good question. I've added a card to the board to look into this.
| @@ -122,7 +123,8 @@ await using (var clientAndService = await clientAndServiceTestCase.CreateTestCas | |||
| } | |||
|
|
|||
| [Test] | |||
| [LatestClientAndLatestServiceTestCases(testNetworkConditions: false, testWebSocket: false)] | |||
| [Timeout(120000)] | |||
There was a problem hiding this comment.
nit: Was there a reason for defining the timeout here? Worth a comment if there is?
There was a problem hiding this comment.
Nicely spotted. That was there when I picked up this PR. Talking to the author, it was likely a temporary thing. Plus, it runs fine on my PC without it. So we decided to deleted it 🙂
[sc-57052]
Background
Web sockets currently do not time out. This is a problem, as it means connections will hang.
Results
Before
Web sockets wrap a
WebSocketcontext, and interact with that link aStream. But they would never timeout the operations, which allowed them to go on forever.After
To ensure timeouts, we wrapped the
WebSocketStreamwithin aNetworkTimeoutStreamto ensure that the operations would indeed time out.JSON Async vs Sync
JSON serialization is not async. So we read off/write to the stream asynchronously, and fall back to 'sync' reading/writing if the payload is too large.
But when this happened, web sockets never timed out (even with
NetworkTimeoutStream). This is because it would used the synchronous versions ofReadandWrite, which theNetworkTimeoutStreamcannot timeout with.To solve this, we introduced the
CallUnderlyingAsyncMethodsStream, so that when JSON called the synchronousReadorWritemethods, they would use the async versions instead. This would allow theNetworkTimeoutStreamto actually time out.WebSocketStream Special Methods
The
IStreamFactory.CreateStreammethod for web sockets returned aWebSocketStream. This made it hard to wrap in the other streams, asWebSocketStreamhad some special methods for doing the initial read and write.To solve this, we simply moved the special methods to
WebSocketExtensionMethods.Tests
We turned on web sockets for all the tests we could (not just the timeout tests).
The only places we did not reenable web sockets were
TimeoutsApplyDuringHandShake. There were issues, so we made a separate card for that.How to review this PR
Quality ✔️
Pre-requisites