GNOME Bugzilla – Bug 724224
gsocketservice: Deactivate the service on accept() error
Last modified: 2018-05-24 16:17:19 UTC
This is a proposed fix for the problem described in the commit message: that GSocketService spews warnings indefinitely if the socket is closed behind its back (or otherwise moves into an error state which means accept() always fails). I’m not entirely sure if this is the best fix, but this is the best approach I can think of given that GSocketService doesn’t have any error reporting to the caller.
Created attachment 268898 [details] [review] gsocketservice: Deactivate the service on accept() error If g_socket_listener_accept() returns an error, as can happen if the socket is closed by another thread (for example), the GSocketService would previously continue to re-queue accept() calls, resulting in an endless stream of ‘fail: *’ warnings on stderr until the GSocketService was manually stopped. Since GSocketService has no error reporting mechanism, the caller could never know that it was spewing warnings, and hence would never stop it. Instead, automatically deactivate the service after the first error, and allow it to be re-started using g_socket_service_start() as normal.
Review of attachment 268898 [details] [review]: This feels vaguely like trying to detect that someone has called free() on my internal state memory... Is it possible that accept() could fail for some transient reason?
(In reply to comment #2) > Review of attachment 268898 [details] [review]: > > This feels vaguely like trying to detect that someone has called free() on my > internal state memory... Yeah, I don’t particularly like the GSocketService API at all. Way too simplistic. > Is it possible that accept() could fail for some transient reason? So, going purely by `man 3 accept` (which I guess is what g_socket_listener_accept() boils down to): • EINTR: accept() interrupted by a signal. • E[N|M]FILE: (process) file descriptor limit hit • ENOBUFS: no buffer space available • ENOMEM: no memory • EPROTO: protocol error (which I guess could be transient) So yes, accept() can fail transiently, so I guess my patch isn’t quite right. Adding a GError would be tricky, and would basically involve turning GSocketService into GSocketListener. How about changing the patch to stop accepting connections if a permanent accept() error occurs, and to continue (but without emitting a warning*) otherwise? *Emitting warnings makes it pretty hard to unit test anything which uses GSocketService since the test can’t statically predict the warnings in order to use g_test_expect_message(). You’d end up having to use g_test_log_set_fatal_handler() to catch and ignore them, which stinks.
(In reply to comment #1) > If g_socket_listener_accept() returns an error, as can happen if the > socket is closed by another thread (for example) So, um, if you're doing that, then stop. GSocket is not documented as being thread-safe, and even if it was, there would still be a race condition here; if you close the socket and then that file descriptor gets reused for something else while the other thread is *not* blocking in poll()/accept(), then once that thread does reach g_main_context_poll() again, it will cheerily end up polling the wrong object via the stale fd, and it may be arbitrarily long before that object polls ready and dispatches the socket source (which can then notice that the GSocket is actually closed and return an error). But anyway, there could be other errors, and GSocketService's behavior here kind of sucks. One thing that your patch doesn't address is that the service can have multiple listening sockets, and you may not want to disable the whole service just because one socket failed. We need to add some API, but I'm not sure exactly what. One possibility is an "accept_failed" signal, though ideally the caller should get access to the GError, and passing a GError* as an argument isn't bindings friendly, so you'd need to have "g_socket_service_get_accept_error(service, &error)" or something that the signal handler could call. And there needs to be some way for the handler to tell the service whether it should close the socket or not...
(In reply to comment #4) > One thing that your patch doesn't address is that the service can have multiple > listening sockets, and you may not want to disable the whole service just > because one socket failed. Whoops, yes. My use case only had one socket, so I completely forgot about that. > We need to add some API, but I'm not sure exactly what. One possibility is an > "accept_failed" signal, though ideally the caller should get access to the > GError, and passing a GError* as an argument isn't bindings friendly, so you'd > need to have "g_socket_service_get_accept_error(service, &error)" or something > that the signal handler could call. And there needs to be some way for the > handler to tell the service whether it should close the socket or not... The alternative is deprecating GSocketService entirely, changing g_socket_listener_accept_async() to automatically (and silently) restart when GSocketListener::changed is emitted, and getting people to replace GSocketService with a loop which yields on g_socket_listener_accept_async() and handles errors as appropriate. As far as I can see, the only non-trivial/-boilerplate part of GSocketService is its handling of changes to the set of sockets in the GSocketListener.
Adjusting component to gio in preparation for GitLab migration
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/828.