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 minimal WebSocket server implementation for evhttp #1322

Merged
merged 43 commits into from Sep 12, 2022
Merged

Add minimal WebSocket server implementation for evhttp #1322

merged 43 commits into from Sep 12, 2022

Conversation

widgetii
Copy link
Member

@widgetii widgetii commented Aug 9, 2022

This adds few functions to use evhttp-based webserver to handle incoming WebSockets connections. We've tried to use both libevent and libwebsockets in our application, but found that we need to have different ports at the same time to handle standard HTTP and WebSockets traffic. This change can help to stick only with libevent library.

Implementation was inspired by modified Libevent source code in ipush project.

Also, WebSocket-based chat server was added as a sample.

@azat
Copy link
Member

azat commented Aug 13, 2022

Thank you! This is a great feature!

Do you have some performance numbers for websocket server?
Also can you please add some unit tests for this great feature?
In the meantime I will review this patch set.

No we have stable CI. So could you please fix building ws-chat-server.c sample for windows?

2022-08-10T09:48:37.9578843Z D:\a\libevent\libevent\sample\ws-chat-server.c(13,10): fatal error C1083: Cannot open include file: 'unistd.h': No such file or directory [D:\a\libevent\libevent\build\ws-chat-server.vcxproj]

@azat azat added the subsystem:http HTTP Related issue label Aug 13, 2022
@azat azat self-assigned this Aug 13, 2022
@widgetii
Copy link
Member Author

I've tried to fix all issues I found, but still stuck with proper unit tests. Could you give me a direction to use them right (internal libevent's kitchen is new for me)?

I added two cases: when bad handshake is provided and a regular WS data exchange. After I read out all headers in http_ws_readcb_hdr it seems that both sides (server and emulated client) are in some kind of deadlock state

test/regress_http.c Outdated Show resolved Hide resolved
@azat
Copy link
Member

azat commented Aug 16, 2022

I've tried to fix all issues I found, but still stuck with proper unit tests. Could you give me a direction to use them right (internal libevent's kitchen is new for me)?

Maybe this will help? https://github.com/libevent/libevent/pull/1322/files#r947026444

@widgetii
Copy link
Member Author

I just skipped including new testes for making all CI process pass (after adding includes for Windows and FreeBSD systems) and now going to revert it. But in the current state WS tests will not pass.

http.c Show resolved Hide resolved
include/event2/ws.h Outdated Show resolved Hide resolved
Copy link
Member

@azat azat left a comment

Choose a reason for hiding this comment

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

Generally LGTM.
Just some minor comments, after they will be addressed I will be happy to merge!
And a minor thing, can you please squash all commits into one and write some descriptions about design decisions that had been made.

event_warn("%s: unexpected frame type %d\n", __func__, type);
evws_force_disconnect_(evws);
}
evbuffer_drain(input, msg_len);
Copy link
Member

Choose a reason for hiding this comment

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

Should this be done in case of incomplete frames?

Copy link
Member Author

Choose a reason for hiding this comment

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

Incomplete frames will not pass through here because of continue operator which do next iteration for the loop

include/event2/ws.h Outdated Show resolved Hide resolved
ws.c Show resolved Hide resolved
include/event2/ws.h Outdated Show resolved Hide resolved
@azat azat merged commit e831308 into libevent:master Sep 12, 2022
@azat
Copy link
Member

azat commented Sep 12, 2022

@widgetii many thanks for you contribution!

@kaend
Copy link

kaend commented Sep 14, 2022

Tor fails to build with this .
Conflict with `SHA1' function from openssl

  CCLD     src/app/tor.exe
D:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: D:/msys64/home/user/zx/lib/libcrypto.a(libcrypto-lib-sha1_one.obj):sha1_one.c:(.text+0x80): multiple definition of `SHA1'; D:/msys64/home/user/zx/lib/libevent.a(sha1.o):sha1.c:(.text+0x16e0): first defined here
collect2.exe: error: ld returned 1 exit status

@widgetii
Copy link
Member Author

There are two options:

  1. Rename our SHA1 into something with different name
  2. Don't use shipped sha1 routines when build with OpenSSL or MbedTLS, but rather use their own implementations

@azat What do you think is better?

@azat
Copy link
Member

azat commented Sep 14, 2022

Let's just rename bundled.

@liudongmiao
Copy link
Contributor

liudongmiao commented Mar 13, 2024

There is no frame type for evws_send, I'd suggest at least two type, text and binary.

This is fixed in master.

@liudongmiao
Copy link
Contributor

I'd suggest the reason of evws_close use enum. And actually, RFC 6445 defines optional message for close frame.

@liudongmiao
Copy link
Contributor

The name of WebSocketFrameType should be avoided. The main code style for enum in libevent seems lower case, and some in rpc is upper case.

And, for the function make_ws_frame, it seems no mask, so, it's only for server response, then it should be renamed to make_server_ws_frame or similar.
If it's only for server response, then header[16] is useless.
Actually, for client, it's max to 14, for server, as there is no mask, it's max to 10.
I'd suggest use union or similar, then use ntohs for 16-bits length and ntohll for 64-bits length.

header[pos++] = (unsigned char)frame_type | 0x80; /* fin */

It should be 0x80 | ((unsigned char)frame_type & 0xf), avoid frame_type for override high bit fin:1 rsv:3.
// In our case, we never use 64 bits length, and in most case, a frame of 16 bits (64K) should be enough.

And, there is no response for ping / pong. Response for pong is useless, however, Response to ping should be handled.

BTW, I use bufferevent_filter_new to handle websocket frame. I love bufferevent_filter_new, specially for BEV_NEED_MORE.

@widgetii
Copy link
Member Author

This PR was merged more than 1,5 years ago and some changes were done on top of it (like differentiations between text and binary data)

If you have any improvements to the code, fell free to open new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
subsystem:http HTTP Related issue
Development

Successfully merging this pull request may close these issues.

None yet

4 participants