Tectu / malloy

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

Possible websocket connection closing bug #39

Closed Tectu closed 3 years ago

Tectu commented 3 years ago

Using malloy-example-server-websocket and connecting to the /timer endpoint does correctly send the 10 messages with 1 second interval. However, it seems like the connection closing is not working as expected.

I am using websocat as a client. On first glance it seems like the connection is not closed properly.

This needs some investigation and possibly fixing.

0x00002a commented 3 years ago

Yeah I ran into the same issue during testing, its an issue with websocat (issue) you need to pass -E

Tectu commented 3 years ago

There still seems to be an issue so I am reopening this.

When running the malloy-example-server-websocket example and using websocat -E the server crashes due to an uncaught exception after sending the 10th message:

C:\Users\joel\Documents\projects\malloy\cmake-build-debug\examples\server\websocket\malloy-example-server-websocket.exe
[2021-07-02 16:37:06.431] [router] [debug] adding websocket endpoint at /
[2021-07-02 16:37:06.432] [router] [debug] adding websocket endpoint at /echo
[2021-07-02 16:37:06.433] [router] [debug] adding websocket endpoint at /timer
[2021-07-02 16:37:06.433] [debug] starting server.
[2021-07-02 16:37:06.434] [debug] starting i/o context.
[2021-07-02 16:40:31.448] [listener] [info] accepting incoming connection from 127.0.0.1:7026
[2021-07-02 16:40:31.449] [http_connection] [debug] launching plain connection.
[2021-07-02 16:40:31.449] [http_connection] [info] upgrading HTTP connection to WS connection.
[2021-07-02 16:40:31.449] [router] [debug] handling WS request: GET /timer
terminate called after throwing an instance of 'boost::wrapexcept<boost::system::system_error>'
  what():  End of file

Process finished with exit code 3
0x00002a commented 3 years ago

Found the bug: https://github.com/Tectu/malloy/blob/9eecc9cc9da9ca22d28323fcaec832df9a974ac0/lib/malloy/websocket/stream.hpp#L75 this calls the non error_code version meaning it will throw on failure. I'm currently writing up an issue meant to be a discussion of how we deal with stuff like this (how to report errors, whether to use error code or exceptions mostly, etc) but this should probably be async anyway.

Tectu commented 3 years ago

40 is definitely something very vital for the future of this library and needs attention by itself.

I am currently unable to check boost.beast sourced/API but I do think that there is an overload that takes an error_code and/or async.

0x00002a commented 3 years ago

Yes there is, I misunderstood the API there, I though the close_reason was filled and reported any errors but its meant to actually have a value (not sure if that might be part of the issue with websocat too). So yeah that code needs just plain fixing :p

Tectu commented 3 years ago

hehe :p are you going to submit a patch or should I put this on my ToDo list?

0x00002a commented 3 years ago

I should have time to do it tomorrow. I'll fix it in the log style and we can potentially change it after #40 is resolved

Tectu commented 3 years ago

Personally I'd put out a fix for this bug before tackling #40 given that at least on the surface this looks like an easy fix.