search

Tectu / malloy

A cross-platform C++20 library providing embeddable server & client components for HTTP and WebSocket.
BSD 3-Clause "New" or "Revised" License
66 stars 8 forks source link

WebSocket: Calling `stop()` when the connection is already closed. #54

Closed Tectu closed 3 years ago

Tectu commented 3 years ago

WebSocket: The current implementation of connection::read() does correctly test for a closed connection. However, it calls connection::stop() when the connection is already closed: https://github.com/Tectu/malloy/blob/218de3b4c63a824f7afdff1e46bc7b9f6390841d/lib/malloy/websocket/connection.hpp#L202

This is in my opinion incorrect as the connection is already closed at that point.

The corresponding boost::beast example simply returns from the read handler: link

@0x00002a Thoughts?

0x00002a commented 3 years ago

Well I definetly added that in, the code I lifted it from seems to be this: https://github.com/Tectu/malloy/blob/a20816b1488824bf185c5a77139c3bbec290b703/lib/malloy/server/websocket/connection/connection.hpp#L200 but I've no idea why, as far as I know that will just error out and the connection is closed anyway so we don't need to do it. We do at least need to set m_state to closed here and add that as a guard in stop() imo.

Tectu commented 3 years ago

Just to be clear communication wise: You agree that calling stop() at that place should be removed? I agree on updating m_state & adding a guard on that in stop(). I'd put that in the same commit.

0x00002a commented 3 years ago

Yes I agree. I am slightly concerned I may have had a good reason for it, but since I can't remember it and it doesn't seem right just by looking at it I probably was just being silly/brain fart :p.