Starscream
Starscream copied to clipboard
Fix connection race when using WSEngine
Issue Link 🔗
Fixes https://github.com/daltoniam/Starscream/issues/791
Goals ⚽
WSEngine is susceptible to connection race which causes an upgrade failure showing inexplicable errors likenotAnUpgrade
and masked and rsv data is not currently supported.
In our environment running the gorilla/websocket server, we found out that the corresponding server-side error was "websocket: client sent data before handshake is complete". That's because it's a security model violation mentioned in RFC 6455.
It would be nice if the library strictly conforms to a WebSocket's security model without users' extra hassle. Because full-blown apps could easily run into the race condition, as they implement a thorough reconnection mechanism triggered at different timing.
How to reproduce 📝
The most straightforward code is below.
socket = WebSocket(request: request)
socket.connect()
socket.connect() # <- Add this line.
Say the first connect() creates a TCPTransport's connection A, and the second() overwrites it by a connection B. Once A's state is updated to ready, Transport wakes up its delegate of WSEngine and WSEngine starts preparing an HTTP upgrade info. However, if overwriting a connection happens before writing it back to Transport, duplicate upgrade info is sent to connection B in succession. Accordingly, it results in two errors consisting of an HTTP server error and a frame processed error.
By the way, NativeEngine doesn't suffer from this issue. It simply creates two connections.
Implementation Details 🚧
To prevent this sort of error, this PR introduces a new property called "isConnecting" and forces WSEngine to start connecting only when the property is false.
I believe this change is the most concise solution under the current base architecture, but let me know how you think.