Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Beast WebSockets #81

Merged
merged 8 commits into from
Mar 6, 2024
Merged

Conversation

Nerixyz
Copy link
Contributor

@Nerixyz Nerixyz commented Nov 22, 2023

I ran into #22, and having to add the termination code isn't ideal, so I added the code here. Since I'm not sure if all Boost installations (that have at least ASIO) will have beast, I added a check if beast is available and didn't include it in the beast/wintls.hpp wrapper (is that assumption true?).

I added tests for sync and async WebSocket servers to the echo test-case.

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Nov 22, 2023

I have no clue how to fix the MinGW tests. I guess I'll need to split them up?

@laudrup
Copy link
Owner

laudrup commented Dec 2, 2023

Thanks a lot for this.

Overall it looks really good.

If I understand this correctly, this implements an overload for boost::beast::teardown required to use this library with beast websockets?

I would definitely like to merge this, but it might make more sense to wait with this before #80 has been completed. There will be conflicts for sure no matter what get merged first though.

I have a few further comments, but please let me know if I've understood correctly that this PR makes use of beast websocket possible by implementing an overload or if I'm missing something.

Thanks.

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Dec 2, 2023

If I understand this correctly, this implements an overload for boost::beast::teardown required to use this library with beast websockets?

Yes, that overload was missing for beast.

I would definitely like to merge this, but it might make more sense to wait with this before #80 has been completed. There will be conflicts for sure no matter what get merged first though.

That's totally fine, I'll watch the PR.

@Nerixyz Nerixyz force-pushed the feat/beast-ws branch 3 times, most recently from abb9191 to a1c0bc3 Compare February 21, 2024 14:17
@Nerixyz
Copy link
Contributor Author

Nerixyz commented Feb 21, 2024

I think you might want to add

concurrency:
  group: unittest-${{ github.ref }}
  cancel-in-progress: true

to the unittest.yml action to stop in-progress actions when updating a PR.

Update: I added it (I can remove it before a merge).

@@ -51,6 +51,10 @@ if(MSVC)
target_compile_options(unittest PRIVATE "-bigobj")
endif()

if(MINGW)
target_compile_options(unittest PRIVATE "-Wa,-mbig-obj")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably not enough for mingw, not sure what to do about it...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the MinGW build fails in the pipeline. I'll have a look at it. Looks like it's not including boost as system headers which is also a bit unfortunate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like adding -O1 works (as in the examples) - I'll push later.

@@ -16,17 +16,18 @@
namespace wintls {
namespace test {

// Note: This is prefixed to avoid conflicts with boost::beast::detail::is_invocable_test
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit unfortunate, even naming the namespace directly in line 44 (: decltype(wintls_is_invocable_test<R>() doesn't work.

@Nerixyz Nerixyz force-pushed the feat/beast-ws branch 3 times, most recently from f8807d2 to 4f17acb Compare February 21, 2024 17:18
@laudrup
Copy link
Owner

laudrup commented Feb 23, 2024

I think you might want to add

concurrency:
  group: unittest-${{ github.ref }}
  cancel-in-progress: true

to the unittest.yml action to stop in-progress actions when updating a PR.

Update: I added it (I can remove it before a merge).

I'm not sure what it does t be honest, but if it cancels builds that are outdated because of new commits then I guess it makes sense to keep it?

include/wintls/beast.hpp Outdated Show resolved Hide resolved
Copy link
Owner

@laudrup laudrup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks great. Thanks for updating this to support the latest changes for standalone ASIO.

I'll have a look at the issues with MinGW if you cannot easily reproduce those (I have MinGW set up in my Windows VM locally).

It would be very cool if you could add an example for how to use this library with websockets. I guess you could more or less copy one of the smallest examples for boost.beast and modify it for use with wintls?

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Feb 23, 2024

I'm not sure what it does t be honest, but if it cancels builds that are outdated because of new commits then I guess it makes sense to keep it?

This ensures that there can only be one active build on a given branch/tag/PR (from github.ref). We use it in another repo where the builds can take some time.

It would be very cool if you could add an example for how to use this library with websockets. I guess you could more or less copy one of the smallest examples for boost.beast and modify it for use with wintls?

I added two examples (from the sync and async examples from beast). I modified the CMakeLists.txt in the examples to make it easier to add examples.

@Nerixyz Nerixyz force-pushed the feat/beast-ws branch 2 times, most recently from 2d0d5ca to 03461a9 Compare February 24, 2024 10:33
@laudrup
Copy link
Owner

laudrup commented Mar 3, 2024

@Nerixyz

It looks really good. Thanks a lot for your effort here. Two things:

Please add the websocket example to doc/examples.rst as well as files for the actual examples (eg. websocket_client.rst). Should hopefully be relative simple to copy and modify the existing examples. You can build the documentation by building the doc target on a non-windows system. If you cannot do that easily I'll have a look before merging.

Also, please split up your PR to separate commits. Something like the change to the Github actions workflows is not really related to the addition of websocket support. The same goes for the change to finding the root for OpenSSL when using MinGW (I have fixed the same in the master branch but your change looks cleaner). So at least three separate commits.

Thanks a lot once again.

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Mar 3, 2024

You can build the documentation by building the doc target on a non-windows system. If you cannot do that easily I'll have a look before merging.

I built it on Windows and it looked fine. Is there a difference when building on non-Windows systems?

So at least three separate commits.

I split it up in a few more commits, but I can squash them if desired.

@laudrup
Copy link
Owner

laudrup commented Mar 6, 2024

You can build the documentation by building the doc target on a non-windows system. If you cannot do that easily I'll have a look before merging.

I built it on Windows and it looked fine. Is there a difference when building on non-Windows systems?

I've only tried to build it on non-Windows systems since it's so much simpler for me. I rarely use Windows for anything :-)

It's very nice to know that the documentation can actually be built on Windows so thanks for that.

So at least three separate commits.

I split it up in a few more commits, but I can squash them if desired.

You might not have to split it into so many commits, but I definitely prefer too many commits than too few (for bisecting, cherry-picking etc.). Thanks a lot.

Looks great. Thanks a lot for all your work on this.

@laudrup laudrup merged commit 0077d73 into laudrup:master Mar 6, 2024
114 checks passed
@Nerixyz Nerixyz deleted the feat/beast-ws branch March 6, 2024 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants