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 732107 - gsocketlistener: Reconsider closing sockets on listener finalisation
gsocketlistener: Reconsider closing sockets on listener finalisation
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: network
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2014-06-23 13:50 UTC by Philip Withnall
Modified: 2014-06-23 16:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gsocketlistener: Mention socket close behaviour in documentation (1.25 KB, patch)
2014-06-23 13:50 UTC, Philip Withnall
none Details | Review
gsocketlistener: Don’t explicitly close sockets on finalisation (1.43 KB, patch)
2014-06-23 14:05 UTC, Philip Withnall
reviewed Details | Review
gsocketlistener: Don’t explicitly close sockets on finalisation (1.98 KB, patch)
2014-06-23 15:55 UTC, Philip Withnall
committed Details | Review

Description Philip Withnall 2014-06-23 13:50:28 UTC
Trivial patch attached.
Comment 1 Philip Withnall 2014-06-23 13:50:30 UTC
Created attachment 279018 [details] [review]
gsocketlistener: Mention socket close behaviour in documentation

Expand the documentation for g_socket_listener_add_socket() to make it
clear that the GSocketListener will close the socket when the listener
is finalised, even if the caller retains a reference to the GSocket.
Comment 2 Philip Withnall 2014-06-23 14:00:16 UTC
Hmm, although, this behaviour is a little counter-intuitive. Perhaps it would be better to remove the explicit g_socket_listener_close() call from g_socket_listener_finalize, and rely on the GSockets closing themselves when their last reference is dropped. That would allow use of temporary GSocketListeners to automatically create IPv4 and IPv6 sockets for a given port (using g_socket_listener_add_inet_port()), the calling code to grab them using g_socket_listener_get_sockets() (bug #584257), then the GSocketListener to be finalised because it isn’t needed any more, and the sockets themselves to live on.

That would not be a behaviour change for sockets added internally by the GSocketListener (because no other code can ever access them to take a reference). It could be a behaviour change for sockets added with g_socket_listener_add_socket(), if the user then finalises the GSocketListener (without explicitly calling g_socket_listener_close()) and expects the sockets to be closed at the same time. I think that’s a bit of a weird case, though.

The GSocketListener documentation never previously guaranteed that GSockets were closed on finalisation, so I think it would be OK to remove the g_socket_listener_close() call from g_socket_listener_finalize().
Comment 3 Philip Withnall 2014-06-23 14:05:17 UTC
Created attachment 279021 [details] [review]
gsocketlistener: Don’t explicitly close sockets on finalisation

Instead of closing the sockets explicitly, let them close themselves
when their final reference is dropped. This makes use of
g_socket_listener_add_socket() more natural.
Comment 4 Dan Winship 2014-06-23 14:58:17 UTC
Comment on attachment 279021 [details] [review]
gsocketlistener: Don’t explicitly close sockets on finalisation

Yes, this is consistent with GSocketConnection (which used to explicitly close its socket on dispose, but now does not).

You should add a comment to g_socket_listener_add_socket() though, both explaining the behavior, and explaining that it didn't work that way before 2.42.

>+   * g_socket_listener_add_socket() was used). */
>   g_ptr_array_free (listener->priv->sockets, TRUE);

nitpick; multi-line comment style in glib generally puts the closing "*/" on its own line.
Comment 5 Philip Withnall 2014-06-23 15:55:09 UTC
Created attachment 279051 [details] [review]
gsocketlistener: Don’t explicitly close sockets on finalisation

Instead of closing the sockets explicitly, let them close themselves
when their final reference is dropped. This makes use of
g_socket_listener_add_socket() more natural.
Comment 6 Philip Withnall 2014-06-23 16:26:42 UTC
Comment on attachment 279051 [details] [review]
gsocketlistener: Don’t explicitly close sockets on finalisation

commit f1095de46f5f43edf494ea6adcabbe86685baadf
Author: Philip Withnall <philip.withnall@collabora.co.uk>
Date:   Mon Jun 23 15:01:28 2014 +0100

    gsocketlistener: Don’t explicitly close sockets on finalisation
    
    Instead of closing the sockets explicitly, let them close themselves
    when their final reference is dropped. This makes use of
    g_socket_listener_add_socket() more natural.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=732107

 gio/gsocketlistener.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)