GNOME Bugzilla – Bug 726375
g_socket_close() does nothing if the socket has had a timeout
Last modified: 2018-02-09 21:22:03 UTC
Created attachment 271948 [details] [review] GSocket: ignore timeout on close Currently, there is a check on "timed_out" at the start of g_socket_close(), this behavior is different from close() and very unexpected, I'm sure lots of apps end up leaking FDs because of that.
Dan's call, but I'm not a huge fan of this approach. I think I'd prefer to have an argument to the check function for ignoring timeouts -- or (without having looked at the code) skip the check function entirely, if that makes sense.
Once you close the connection, any previous timeout state is no longer relevant, so arguably it's correct for close() to clear priv->timed_out. OTOH, once the socket is closed, priv->timed_out doesn't matter any more, because check_socket() will always return G_IO_ERROR_CLOSED... But close() isn't the only case where you don't care about priv->timed_out. priv->timed_out means a GSocketSource timed out (with the implicit expectation that when the source triggers, you'd immediately make the call you were waiting to make, and get back the TIMED_OUT error at that point). So only calls you can poll a GSocketSource for should check priv->timed_out. I think this means g_socket_send*(), g_socket_receive*(), g_socket_accept(), g_socket_check_connect_result(), and g_socket_condition_*(). Everything else should either ignore it or clear it. I think I'd vote for splitting the timeout check into a separate check_timeout(), and calling that from the places that care. Also, there are a few missing check_socket() calls (eg, g_socket_get/set_option())
Created attachment 272709 [details] [review] GSocket: ignore timed out state when not relevant Do this by separating the timeout check from the other socket checks.
Created attachment 272710 [details] [review] GSocket: Check that socket has been initialized where missing g_socket_get_credentials() and g_socket_create_source() were missing the check. Adding the checks to get/set_options and get_available_bytes() caused a bunch of unit tests to fail. I wouldn't be suprised if that indicated that the unit tests are broken somehow, but I haven't had time to investigate.
Comment on attachment 272709 [details] [review] GSocket: ignore timed out state when not relevant >+check_timeout (GSocket *socket, >+ GError **error) fix the second line to use only spaces and no tabs please. (We're trying to be consistent about that with new code.) Otherwise good.
Comment on attachment 272710 [details] [review] GSocket: Check that socket has been initialized where missing >@@ -3436,6 +3436,9 @@ g_socket_create_source (GSocket *socket, > { > g_return_val_if_fail (G_IS_SOCKET (socket) && (cancellable == NULL || G_IS_CANCELLABLE (cancellable)), NULL); > >+ if (!check_socket (socket, NULL)) >+ return NULL; Hm... g_socket_create_source() can't currently return NULL except on programmer error. I think in this case probably it should return a valid source, but then source should then immediately trigger with an error condition. > Adding the checks to get/set_options and get_available_bytes() caused a bunch > of unit tests to fail. I wouldn't be suprised if that indicated that the unit > tests are broken somehow, but I haven't had time to investigate. ok, just commit the first patch for now then and I'll look into this
Comment on attachment 272709 [details] [review] GSocket: ignore timed out state when not relevant Committed the first one: commit 0c65f7e45a29fa93b0e24488ef881d374d621541 Author: Olivier Crête <olivier.crete@collabora.com> Date: Fri Mar 14 17:46:38 2014 -0400 GSocket: ignore timed out state when not relevant Do this by separating the timeout check from the other socket checks. https://bugzilla.gnome.org/show_bug.cgi?id=726375
Well this seems to be fixed. I reported bug #793343 for the missing check_socket() calls.