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

Fix crash on ws close #43

Closed 0x00002a closed 3 years ago

0x00002a commented 3 years ago

Fixes: #39

Note this is currently not clang-formatted since I don't think thats been done yet, so the clang-format check will fail

0x00002a commented 3 years ago

Okay so mingw + tls fails at this with boost 1.74.0 due to "file too big". I'm not sure what tripped it, I assume it must be the callback to async_close + the ssl_stream stuff. I'm really not sure what to do about this, since main.cpp is going to have to include the definitions no matter. I could try switching to std::function rather than template callback but that has the type-erasure overhead.

I'm a bit busy today so I may not get back to this for a few hours but any thoughts @Tectu?

Tectu commented 3 years ago

I'm a bit busy today so I may not get back to this for a few hours but any thoughts @Tectu?

I don't know... I really really don't know. I do understand that this library is pushing file sizes - especially with underlying boost.beast. I mean the boost.beast examples themselves already don't compile without the corresponding big-obj option(s) so adding stuff on top like malloy does is certainly not improving the situation - but it's necessary 🙈.

As you mentioned main.cpp is going to have to include all the necessary definitions no matter how much we shuffle things around.

0x00002a commented 3 years ago

From the string that apparently caused the overflow I would guess this is mostly a beast issue. Looks like the typename of the asio executor for the ssl websocket stream is simply too much. Just a guess though. I wonder, does it work with -Os?

I consider this a bug in mingw tbh. MSVC can compile it fine, and so can gcc (but that's a different format)

Tectu commented 3 years ago

I consider this a bug in mingw tbh. MSVC can compile it fine, and so can gcc (but that's a different format)

Yeah... I've been (forcefully) working for the better part of a decade with MinGW on Windows. It's just a big pain. The projects that consume malloy could be changed to a clang based toolchain. However, clang 12.0 doesn't seem to provide enough C++20-ness to compile malloy (I didn't investigate properly, just switched to clang and got a bunch of lambda errors etc.

0x00002a commented 3 years ago

Yeah looks like clang 12 doesn't properly support some part of concepts: godbolt. trunk can manage though, so I guess the CI could build from source if we did that, although I'm guessing thats not really an acceptable requirement to put on users :p.

Edit We actually don't need that to be a lambda: https://godbolt.org/z/jT8vjE8b5. I'll see what I can do about the clang 12 compile today, hopefully I can get it working :). It doesn't like the ranges include but as far as I can see its not being used anyway?

Tectu commented 3 years ago

I wanted to pull your branch to build this locally. This needs some updating/merging tho. Should I handle that or are you by any chance highly motivated to handle this? :p

0x00002a commented 3 years ago

I just did a quick merge in githubs web editor. Feel free to force push it if you've done it locally though, I don't mind :p

Tectu commented 3 years ago

Force pushing is not something I like to do if avoidable in pretty much any situation as a human being :p

+1 lazy points for using the web editor tho :p

0x00002a commented 3 years ago

Well thats what you get for being lazy I guess, missed the fact that underlying_conn_ had been renamed. now it should work :p

Tectu commented 3 years ago

Not my fault that you don't adhere to the coding style guidelines 7098f8eb0563d279bf29be0407e2269cb25b50c4 😝😜