any1 / neatvnc

A liberally licensed VNC server library with a clean interface
ISC License
118 stars 29 forks source link

Embedding libuv event loop causes issues #16

Closed agners closed 4 years ago

agners commented 4 years ago

Thanks for the library! I used it in a VNC Weston backend, see: https://gitlab.freedesktop.org/wayland/weston/merge_requests/362

I do have troubles embedding the libuv event loop into Weston (which makes use of the Wayland event loop. This is (probably) not an issue of neatvnc but maybe you do have a hint/idea. I am not sure why embedding does not work, from by understanding using uv_backend_fd() and uv_run(..., UV_RUN_NOWAIT) should do the job, but with only that approach I do not get an event. When I sprinkle some more uv_run(..., UV_RUN_NOWAIT) it starts to work, but that is definitely not how it should be.

What would also be nice if we could make neatvnc event loop library agnostic somehow.

any1 commented 4 years ago

It's nice to see people using this.

According to the libuv documentation you should actually check the return value from uv_run() to see if you need to run it again.

http://docs.libuv.org/en/v1.x/loop.html#c.uv_run:

UV_RUN_NOWAIT: Poll for i/o once but don’t block if there are no pending callbacks. Returns zero if done (no active handles or requests left), or non-zero if more callbacks are expected (meaning you should run the event loop again sometime in the future).

I'm not really particularly fond of libuv after having used it now for a while, but it was good enough when I was getting started with this. Had I decided to do my own event handling, I probably would have gotten carried away with writing an event loop library instead! Now that the project has matured a bit, I feel like I might want to replace libuv with something bespoke, for a better fit. So, yes, we can make it event loop library agnostic.

agners commented 4 years ago

According to the libuv documentation you should actually check the return value from uv_run() to see if you need to run it again.

I did try that, basically

while (uv_run(..., UV_RUN_NOWAIT));

However, this ended up looping forever. So it seems that libuv always has an event it needs to handle, but it does not get handled properly...? I will have to investigate further.

Now that the project has matured a bit, I feel like I might want to replace libuv with something bespoke, for a better fit. So, yes, we can make it event loop library agnostic.

Do you have a rough idea/something particular in your mind?

any1 commented 4 years ago

However, this ended up looping forever. So it seems that libuv always has an event it needs to handle, but it does not get handled properly...? I will have to investigate further.

This could be an issue with neatvnc, i.e. there might be some event that's left pending. However, that should cause epoll_wait() or poll() to return immediately and causing high CPU load. I don't think that this function in libuv is very portable. E.g. if you only have poll() or select() on your system libuv would have to spawn a thread and use use pipe() for that fd. I suspect it doesn't really support uv_backend_fd() at all in that case. Maybe strace can help.

Now that the project has matured a bit, I feel like I might want to replace libuv with something bespoke, for a better fit. So, yes, we can make it event loop library agnostic.

Do you have a rough idea/something particular in your mind?

A portable solution requires complexity within the library or in the interface. I'm not sure which to choose. We could have a pair of nvnc_get_fd() and nvnc_dispatch() or the option to register callbacks for adding, removing and modifying file descriptor watchers and creating timers. The former requires spawning a thread for working with poll() and the latter is harder on the user.

agners commented 4 years ago

However, this ended up looping forever. So it seems that libuv always has an event it needs to handle, but it does not get handled properly...? I will have to investigate further.

This could be an issue with neatvnc, i.e. there might be some event that's left pending. However, that should cause epoll_wait() or poll() to return immediately and causing high CPU load. I don't think that this function in libuv is very portable. E.g. if you only have poll() or select() on your system libuv would have to spawn a thread and use use pipe() for that fd. I suspect it doesn't really support uv_backend_fd() at all in that case. Maybe strace can help.

stracing when using while (uv_run(..., UV_RUN_NOWAIT)); I get continuously this:

...
epoll_wait(11, [], 1024, 0)             = 0
epoll_wait(11, [], 1024, 0)             = 0
...

Event handling of Neat VNC works in that case, but Weston itself does not handle any events anymore (including signal handling which is handled by wl_event_loop as well, so Ctrl+C does not work anymore). 11 is the fd returned by uv_backend_fd()...

Now that the project has matured a bit, I feel like I might want to replace libuv with something bespoke, for a better fit. So, yes, we can make it event loop library agnostic.

Do you have a rough idea/something particular in your mind?

A portable solution requires complexity within the library or in the interface. I'm not sure which to choose. We could have a pair of nvnc_get_fd() and nvnc_dispatch() or the option to register callbacks for adding, removing and modifying file descriptor watchers and creating timers. The former requires spawning a thread for working with poll() and the latter is harder on the user.

The former seems more appealing to me, from what I understand that would be rather trivial to integrate into Weston. But would that be sufficient for all usages of libuv in the library currently? E.g. how would you handle uv_queue_work?

I guess Neat VNC still could provide an example using libuv to make it easier for users with no specific requirements from their end.

any1 commented 4 years ago

stracing when using while (uv_run(..., UV_RUN_NOWAIT)); I get continuously this:

...
epoll_wait(11, [], 1024, 0)             = 0
epoll_wait(11, [], 1024, 0)             = 0
...

Huh, this might be a libuv bug after all. Have you tried running with libuv's master branch?

E.g. how would you handle uv_queue_work?

Well, I guess I'd just implement my own thread pool. I don't like that I can only change the number of worker threads in libuv via an environment variable anyway. I'd want to have the number of threads match the number of cores by default.

any1 commented 4 years ago

If you've been compiling without TLS, the newest commit might fix your problem.

agners commented 4 years ago

@any1 just looked into this again. I am pretty sure I have been compiling with TLS back then.

Tried with today's master with and without TLS, no luck so far. I will look into updating libuv.

any1 commented 4 years ago

Neat VNC now uses aml.