After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 724224 - gsocketservice: Deactivate the service on accept() error
gsocketservice: Deactivate the service on accept() error
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: gio
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2014-02-12 11:02 UTC by Philip Withnall
Modified: 2018-05-24 16:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gsocketservice: Deactivate the service on accept() error (1.86 KB, patch)
2014-02-12 11:02 UTC, Philip Withnall
needs-work Details | Review

Description Philip Withnall 2014-02-12 11:02:47 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.
Comment 1 Philip Withnall 2014-02-12 11:02:49 UTC
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.
Comment 2 Allison Karlitskaya (desrt) 2014-02-12 13:39:14 UTC
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?
Comment 3 Philip Withnall 2014-02-13 11:43:29 UTC
(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.
Comment 4 Dan Winship 2014-02-13 13:54:11 UTC
(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...
Comment 5 Philip Withnall 2014-02-18 00:00:39 UTC
(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.
Comment 6 Michael Catanzaro 2018-03-21 00:48:18 UTC
Adjusting component to gio in preparation for GitLab migration
Comment 7 GNOME Infrastructure Team 2018-05-24 16:17:19 UTC
-- 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.