Starscream icon indicating copy to clipboard operation
Starscream copied to clipboard

Fix connection race when using WSEngine

Open yoheimuta opened this issue 3 years ago • 0 comments

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.

yoheimuta avatar Oct 19 '21 07:10 yoheimuta