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 710700 - glib-networking: treat G_IO_ERROR_TIMED_OUT as EINTR
glib-networking: treat G_IO_ERROR_TIMED_OUT as EINTR
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: network
2.38.x
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2013-10-23 07:08 UTC by Aleix Conchillo Flaqué
Modified: 2013-11-17 16:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
G_IO_ERROR_TIMED_OUT as EINTR (1.21 KB, patch)
2013-10-23 07:13 UTC, Aleix Conchillo Flaqué
reviewed Details | Review
G_IO_ERROR_TIMED_OUT as EINTR plus test (14.01 KB, patch)
2013-10-24 21:23 UTC, Aleix Conchillo Flaqué
reviewed Details | Review
G_IO_ERROR_TIMED_OUT as EINTR plus test (14.01 KB, patch)
2013-10-25 14:19 UTC, Aleix Conchillo Flaqué
committed Details | Review

Description Aleix Conchillo Flaqué 2013-10-23 07:08:31 UTC
Currently, if the GnuTLS pull function is cancellable and G_IO_ERROR_TIMED_OUT is returned, the gnutls transport error is set to EIO and which causes GnuTLS to invalidate the session.

EINTR should be used instead, so the session is kept alive.

A use case is the always opened RTSP connection that is kept between the RTSP server and the client (at least with the rtspsrc GStreamer client). No messages might be sent from the server so the client sends a keep alive message after a timeout.
Comment 1 Aleix Conchillo Flaqué 2013-10-23 07:13:36 UTC
Created attachment 257894 [details] [review]
G_IO_ERROR_TIMED_OUT as EINTR
Comment 2 Dan Winship 2013-10-24 02:13:17 UTC
Comment on attachment 257894 [details] [review]
G_IO_ERROR_TIMED_OUT as EINTR

I think you also need to update end_gnutls_io() to not call class->failed() in this case, as with G_IO_ERROR_WOULD_BLOCK.

It would also be good to add a test to tls/tests/connection.c for this...
Comment 3 Aleix Conchillo Flaqué 2013-10-24 21:23:50 UTC
Created attachment 258059 [details] [review]
G_IO_ERROR_TIMED_OUT as EINTR plus test

I just added your suggestion and created a new test "read-time-out-then-write".

Without the fix, the test fails with:

GLib-Net:ERROR:connection.c:804:socket_client_timed_out_write: assertion failed (error == NULL): Error writing data to TLS socket: The specified session has been invalidated for some reason. (g-tls-error-quark, 1)
Comment 4 Dan Winship 2013-10-25 14:08:20 UTC
Comment on attachment 258059 [details] [review]
G_IO_ERROR_TIMED_OUT as EINTR plus test

> Without the fix, the test fails with:
> 
> GLib-Net:ERROR:connection.c:804:socket_client_timed_out_write: assertion failed
> (error == NULL): Error writing data to TLS socket: The specified session has
> been invalidated for some reason. (g-tls-error-quark, 1)

Oh, cool. There's another bug about connections randomly failing with "session has been invalidated for some reason" that I could never figure out... I bet it's also caused by the use of timeouts.

>  G_IO_ERROR_TIMED_OUT as EINT instead of EIO. Long time connections
                               ^ missing "R"

>+  /* Close the server now */
>+  close_server_connection (test);

why do you need to do this?
Comment 5 Aleix Conchillo Flaqué 2013-10-25 14:16:50 UTC
(In reply to comment #4)
> 
> Oh, cool. There's another bug about connections randomly failing with "session
> has been invalidated for some reason" that I could never figure out... I bet
> it's also caused by the use of timeouts.
> 

I don't know about the other bug, but that would be nice, yes.

> >  G_IO_ERROR_TIMED_OUT as EINT instead of EIO. Long time connections
>                                ^ missing "R"
> 

Commit log fixed. Sending another patch.

> >+  /* Close the server now */
> >+  close_server_connection (test);
> 
> why do you need to do this?

Otherwise the test doesn't finish. Before this test, the server is always closed after sending TEST_DATA. Now, it's optional and for this particular test I keep the server open so I can send more data after a timeout occurs.
Comment 6 Aleix Conchillo Flaqué 2013-10-25 14:19:39 UTC
Created attachment 258127 [details] [review]
G_IO_ERROR_TIMED_OUT as EINTR plus test

Typo in commit log fixed.
Comment 7 Dan Winship 2013-11-17 16:28:50 UTC
committed to master. thanks for the fix