-
-
Notifications
You must be signed in to change notification settings - Fork 345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix client Websockets (broken) #2749
Conversation
cb508a7
to
337eaa8
Compare
(Discovered memory leak in `WebsocketConnection::send` - SmingHub#2749)
WebsocketConnection::send
releases source stream on failureWebsocketConnection::send
leak and close failure
WebsocketConnection::send
leak and close failureWebsocketConnection::send
leak and close failure
WebsocketConnection::send
leak and close failureWebsocketConnection::send
leak and close failure
WebsocketConnection::send
leak and close failureWebsocketConnection::send
leak and close failure
WebsocketConnection::send
leak and close failureWebsocketConnection::send
leak and close failure
Changing to WIP as there appear to be further issues with client websockets. Am testing with local websocket server and it's thrown up some additional problems I'm resolving. |
60fc58b
to
aa98304
Compare
WebsocketConnection::send
leak and close failureWebsocketConnection::send
leak and close failure
OK, seems to behave itself much better now. |
WebsocketConnection::send
leak and close failureThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also needs rebase. And running through valgrind again...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mask
must be unique_ptr.
aa98304
to
6715ab2
Compare
Yet more bugs... this is driving me potty! Two things:
I suspect 2 and 3 are related. Something off in |
6715ab2
to
895ade0
Compare
@slaff OK, reception now working. This has turned out to be a larger PR than I first anticipated. Also broken in 4.2.2 LTS. |
Testing with websocket server to make sure that's not broken... |
895ade0
to
474ad75
Compare
474ad75
to
d30e937
Compare
Add `wsserver` build target with simple echo server Use local server by default (remote website no longer exists) Add `test.py` script to check connection protocol Update sample to send both text and binary messages
Websocket client passes local mask data which can get corrupted, leading to unreliable messages.
…ction == nullptr
HttpConnection is NOT auto-self-destruct so we need to change that during closure.
`HttpClientConnection` resets our `receive` delegate
d30e937
to
031ce6d
Compare
Can either be in response to a received CLOSE, or as an outgoing notification. Only difference is in the status code sent.
031ce6d
to
47d7fa2
Compare
This PR addresses several issues discovered in the client websocket stack. A simple local websocket echo server has been added for testing, which can be run using
make wsserver
.Memory leaks
Running
HttpServer_Websockets
sample with valgrind shows a memory leak (details below, fig 1).I can see from the logic of
WebsocketConnection::send
that there are multiple reasons the call could fail, but thesource
stream is only destroyed in one of them.Failed connection
Testing with the local server failed with
websockets.exceptions.InvalidHeaderValue: invalid Sec-WebSocket-Key header
.The key was 17 bytes instead of 16.
utf-8 decoding errors
Turns out message was getting corrupted because mask value passed to XorStream is on stack, which then gets overwritten before message has been sent out. Fixed by taking a copy of the value.
CLOSE message not being sent
Tested with
Websocket_Client
sample (running local echo server) to confirm correct behaviour, noticed aStreams without known size are not supported
message when closing the connection. This blocked sending 'CLOSE' notifications which have no payload.Issues during CLOSE
The TCP socket was being closed too soon, causing additional errors. Increasing timeout to 2 seconds fixes this. Also included a status code in the CLOSE message indicating normal closure; this is optional, but seems like a good thing to do.
RFC 6455 states: The application MUST NOT send any more data frames after sending a Close frame. If an endpoint receives a Close frame and did not previously send a Close frame, the endpoint MUST send a Close frame in response.
Therefore, the
close()
logic has been changed so that a CLOSE message is always sent, either in response to a previous incoming request (in which case the received status is echoed back) or as notification that we're closing (status 1000 - normal closure). Checked server operation withHttpServer_Websockets
sampleSimplify packet generation
It's not necessary to pre-calculate the packet length as it's never more than 14 bytes in length.
WebsocketConnection not getting destroyed
HttpConnection objects are not 'auto-released' so leaks memory every time a WebsocketConnection is destroyed (512+ bytes). Simplest fix for this is to add a
setAutoSelfDestruct
method toTcpConnection
so this can be changed.Messages not being received
Connection is activated OK, but
HttpClientConnection
then callsinit
in itsonConnected
handler which resets the newreceive
delegate. This causes incoming websocket frames to be passed to the http parser, instead of the WS parser, hence theHTTP parser error: HPE_INVALID_CONSTANT
message.====
Fig 1: Initial memory leak reported by valgrind