-
Notifications
You must be signed in to change notification settings - Fork 237
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
[web] Support libevent as WS server instead of libwebsockets #1818
base: master
Are you sure you want to change the base?
Conversation
If libevent >= 2.2 is detected during configure and "websocket_port" == 0 in the config file, the libwebsocket implementation is disabled and instead the libevent http server offers the websocket connection. The connection to the websocket is then done with the path "/ws".
Sounds really promising. Too bad that the libevent team is slow at releasing versions of their library; last commit was last year. |
Yes, more frequent releases of libevent would be nice. Especially because it also takes quite a long time until all the distros pick up the new release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a few comments and suggestions. I think it should work with the multithreaded http server, at least as long as THREADPOOL_NTHREADS is 1, because then there is still only one httpd thread and one evhttp instance.
[event_base_new], [event2/event.h]) | ||
[event_base_new], [event2/event.h], | ||
[dnl check for version 2.2 (with websocket server support) | ||
PKG_CHECK_EXISTS([libevent >= 2.2.1], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to do a feature check instead of a version check?
return json_response; | ||
} | ||
|
||
/* Thread: library (the thread the event occurred) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would also be called from other threads than the library thread
struct client *client = NULL; | ||
char *reply = NULL; | ||
|
||
for (client = clients; client; client = clients->next) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look thread safe to me since clients is a global? And is evws_send_text safe to call with ->evws, which looks like it belongs to the httpd thread's evhttp?
return -1; | ||
} | ||
|
||
DPRINTF(E_DBG, L_WEB, "notify callback request: %s\n", json_object_to_json_string(request)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest commenting this out, it's usually quite "noisy"
|
||
evhttp_set_cb(evhttp, "/ws", gencb_ws, NULL); | ||
|
||
listener_add(listener_cb, LISTENER_UPDATE | LISTENER_DATABASE | LISTENER_PAIRING | LISTENER_SPOTIFY | LISTENER_LASTFM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should have a LISTENER_ALL?
@@ -38,6 +38,7 @@ | |||
#include "logger.h" | |||
#include "commands.h" | |||
#include "httpd_internal.h" | |||
#include "websocket_ev.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
websocket_ev.c isn't that much code, and since it's tied to libevent's evhttp and not called from anywhere else, you could just add it here in httpd_libevent.c?
DPRINTF(E_DBG, L_WEB, "notify callback reply: %d\n", events); | ||
|
||
notify = json_object_new_array(); | ||
if (events & LISTENER_UPDATE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A map might be a good idea, could then also be used in process_notify_request
The next planned minor release 2.2 of libevent will have support for running a websocket server. I thought, I give it a try and see if it works in OwnTone and check if it is something worth adding :-)
The changes in this PR add a libevent based websocket server, that accepts connections under the path
/ws
(and the same port as the web interface).If libevent >= 2.2 is detected during configure and "websocket_port" == 0 in the config file, the libwebsocket implementation is disabled and instead the libevent websocket server is active.
The web interface was updated to attempt a WS connection to
/ws
if websocket_port == 0.The advantages in using libevent are - if it proves to be reliable - that the libwebsocket dependency could be removed. Additionally, only having one port for the web interface, will make it easier to use a reverse proxy with OwnTone (especially if the reverse proxy is running on the same host).
I did some short tests, and it seems to be working fine, but more tests are required.
I tested it locally and by building an OwnTone container with libevent from git-master (https://github.com/chme/owntone-container/tree/feat/libevent-git). Also I am unsure how it will work with the multi threading support of the http server in OwnTone.
Currently only an alpha release of libevent 2.2 is available (https://github.com/libevent/libevent/releases/tag/release-2.2.1-alpha) . Note, that the libevent 2.2.1 alpha release contains a bug that prevents ws connections from Firefox (Chrome is working fine). The bug is already fixed in the master branch of libevent.