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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use wslay as a WebSocket library #30263

Merged
merged 7 commits into from Jul 4, 2019

Conversation

@Faless
Copy link
Contributor

commented Jul 2, 2019

I know, I know, another WIP, sorry! 馃檹
I have a few intertwined PRs, this is probably next in line, then I'll move back to the Crypto one.

Why WIP? There are some edge cases when the client gets disconnected it does not emit the appropriate signal. I'm working on it. (plus few TODOs, mostly optimizations).

EDIT: No longer WIP, ready fro review.

Beside that both client and server are working. Client also support SSL (server should be able to do that easily as soon as the Crypto PR is in).

While I'm pretty happy with this work, I'm sure it's still not as stable as the other library, mainly because this Godot integration is less tested then the other.

It would be great if people who uses the WebSocket module could give this PR a try with their servers to verify I didn't mess up the handshake part ( 鉂わ笍 ).

Please report any issue you might encounter, I'll try to fix the edge cases mentioned above in the coming days.

Fixes #27632 (given the libwebsocket is dropped, and the new lib uses our StreamPeerSSL implementation).
Sadly, while the new library do include a .pc file, it does not seem to be included in Debian/Ubuntu packages, not sure about other distros.

@Faless Faless requested review from akien-mga and godotengine/documentation as code owners Jul 2, 2019

@akien-mga akien-mga added this to the 3.2 milestone Jul 2, 2019

@Faless Faless force-pushed the Faless:ws/wslay_pr branch from ef44587 to 505ad4d Jul 3, 2019

COPYRIGHT.txt Show resolved Hide resolved

@Faless Faless force-pushed the Faless:ws/wslay_pr branch 4 times, most recently from 37df4dc to 9d7a933 Jul 3, 2019

@Faless Faless changed the title [WIP] Use wslay as a WebSocket library Use wslay as a WebSocket library Jul 3, 2019

COPYRIGHT.txt Outdated Show resolved Hide resolved
COPYRIGHT.txt Outdated Show resolved Hide resolved
@akien-mga

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Sadly, while the new library do include a .pc file, it does not seem to be included in Debian/Ubuntu packages, not sure about other distros.

I'll package it for Mageia to give it a try, and if that works I'll import it in Fedora too.

Edit: It actually is packaged in Debian/Ubuntu already in recent versions, as well as openSUSE: https://pkgs.org/download/wslay
They're not all on 1.1.0 though so not sure if they'd build fine.

@akien-mga

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

I added a conflict in the SCsub as I replaced all CPPFLAGS by CPPDEFINES.

@Faless Faless force-pushed the Faless:ws/wslay_pr branch from 9d7a933 to 5f311e9 Jul 3, 2019

if not env['builtin_libwebsockets']:
env.ParseConfig('pkg-config libwebsockets --cflags --libs')
if not env['builtin_wslay']:
env.ParseConfig('pkg-config libwslay --cflags --libs')

This comment has been minimized.

Copy link
@akien-mga

akien-mga Jul 3, 2019

Member

Just for the reference (no need to change anything for now), the libwslay.pc pkgconfig file is not a given even on distros which package the library. wslay comes with two buildsystems, autotools and cmake, and only the former configures and installs the .pc file.

openSUSE use the autotools buildsystem and have thus libwslay.pc installed, so this will work.
Debian/Ubuntu use the CMake buildsystem, <troll>which, like any good CMake project, is CMake-centric and only installs its own .cmake config files</troll>. I'll open a ticket on https://salsa.debian.org/debian/wslay to suggest that they add a pkgconfig file. Edit: Done, https://salsa.debian.org/debian/wslay/issues/1

For Mageia I used the CMake buildsystem, and wrote a libwslay.pc file myself. I use the same patch as Debian for the CMakeLists.txt to get a shared library with proper soname and libwslay.so, instead of libwslay_shared.so (ugh).

@akien-mga

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Builds fine locally on Mageia 7 x86_64. I tested both against the builtin version (default) and against the system-provided wslay package that I just packaged for Mageia :)

@akien-mga

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

@AshfordN , @RyanJEC, would you be able to test this PR on your WebSockets projects to ensure that there are no regressions compared to libwebsockets 3.0.1?

@Faless

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

@LikeLakers2 too could be interested

@LikeLakers2

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

I don't currently have any projects I'm working on that use websockets (the only one I've done so far, a discord library, I'm thinking of remaking from scratch anyways; that one will be a ways off though), but I do have a couple ideas for ones. I might as well test it when I get the chance.

@RyanJEC

This comment has been minimized.

Copy link

commented Jul 4, 2019

Seems to be working with my systems. Found no major hitches...Yet

Could we get an example on how to use WebSocketServer with SSL when it's ready.

Faless added 4 commits Jun 24, 2019
WebSocket module now uses wslay library.
Both client and server are supported on native builds (as usual).

SSL server is still not supported, but will soon be possible with this
new library.

The API stays the same, we just need to work out potential issues due to
this big library switch.

@Faless Faless force-pushed the Faless:ws/wslay_pr branch from 5f311e9 to 3380dc9 Jul 4, 2019

@Faless

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Could we get an example on how to use this with SSL when it's ready.

Sure, client already works as before (just pass wss://mydomain.com, i.e. wss instead of ws).

Server will likely have two new parameters to listen e.g. listen(port, protocols, gd_mp_api=false, key=null, cert=null). When they are not null, SSL will be used. It should be quite straight forward after the crypto PR.

I'll make sure to implement it in the websocket_chat_demo and push it to the godot demo projects repository.

Thank you for trying this out!

@akien-mga akien-mga merged commit 550f436 into godotengine:master Jul 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akien-mga

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

Thanks a lot, awesome work! :)

@akien-mga

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

For the reference, if anyone wants to cherrypick this PR for their own 3.1 branch, you'll need #30419 too to keep feature parity.

@Faless

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

And also #30434

@djole1973

This comment has been minimized.

Copy link

commented Jul 19, 2019

why did you change/remove libwebsocket?

@Faless

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

why did you change/remove libwebsocket?

because it was bloated, broken, unmaintainable.
If you check the line changes, it's +4,052 鈭49,109. This already gives you a picture of how bad that library was for our needs.
Each library update changed the file structure completely and tended to break stuff (see #27560), so much that we had to roll back (#28568).
We couldn't use our socket implementation (i.e. the library do not support I/O abstraction) and the developer clearly stated he had no intention in accepting work in that direction.
The SSL support was troublesome due to their awful wrapper (#27632),
in general it's a badly written library, and the developer seemed more interested in implementing non-standard features instead of fixing existing issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.