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

[web] Support libevent as WS server instead of libwebsockets #1818

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chme
Copy link
Collaborator

@chme chme commented Oct 3, 2024

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.

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".
@ejurgensen
Copy link
Member

This looks great, I think it would be very good to have an alternative to libwebsockets. Maybe this can also be a way of solving issue #1699 and the problem mentioned in #1446.

I will try to check the http multithreading to see if this could cause any problems.

@hacketiwack
Copy link
Collaborator

Sounds really promising. Too bad that the libevent team is slow at releasing versions of their library; last commit was last year.

@chme
Copy link
Collaborator Author

chme commented Oct 6, 2024

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.

Copy link
Member

@ejurgensen ejurgensen left a 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],
Copy link
Member

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) */
Copy link
Member

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)
Copy link
Member

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));
Copy link
Member

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
Copy link
Member

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"
Copy link
Member

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)
Copy link
Member

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

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.

3 participants