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 790436 - websocket-connection: do not send new frames until the previous is not successfully sent
websocket-connection: do not send new frames until the previous is not succes...
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: API
unspecified
Other All
: Normal normal
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
Depends on: 792862
Blocks:
 
 
Reported: 2017-11-16 08:51 UTC by Ignacio Casal Quinteiro (nacho)
Modified: 2018-02-01 08:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
websocket-connection: do not send new frames until the previous is not successfully sent (1.93 KB, patch)
2017-11-16 08:51 UTC, Ignacio Casal Quinteiro (nacho)
none Details | Review
websocket-connection: do not send new frames until the previous is not successfully sent (1.88 KB, patch)
2018-01-24 10:24 UTC, Ignacio Casal Quinteiro (nacho)
committed Details | Review

Description Ignacio Casal Quinteiro (nacho) 2017-11-16 08:51:17 UTC
If the sending of a frame fails with G_IO_ERROR_WOULD_BLOCK,
we must send it again before sending more urgent ones.
This change is relevant in case a SSL connection is being used
because SSL expects the same message to be resent
Comment 1 Ignacio Casal Quinteiro (nacho) 2017-11-16 08:51:25 UTC
Created attachment 363800 [details] [review]
websocket-connection: do not send new frames until the previous is not successfully sent
Comment 2 David Woodhouse 2017-11-16 09:17:58 UTC
This is a horrid OpenSSL API restriction, right? It is... suboptimal that this needs to permeate the GPollableOutputStream semantics (and various other APIs) too.

But if we really can't make it go away, we need to start with *documenting* this requirement for all APIs that now have it. Don't just work around it in *one* user by tribal knowledge. Maybe enforce it, at a higher level, in the affected APIs too — when returning G_IO_ERROR_WOULD_BLOCK, *check* the next thing sent to ensure it's identical. Even when a saner TLS library (or no TLS) is actually the back end. Depending on how you phrase the requirement when you document it, I suppose...
Comment 3 Dan Winship 2017-11-16 12:21:21 UTC
> This is a horrid OpenSSL API restriction, right? It is... suboptimal
> that this needs to permeate the GPollableOutputStream semantics
> (and various other APIs) too.

I think gnutls has the same problem: bug 739983
Comment 4 Dan Winship 2017-11-16 12:22:18 UTC
Comment on attachment 363800 [details] [review]
websocket-connection: do not send new frames until the previous is not successfully sent

seems right... probably?
Comment 5 David Woodhouse 2017-11-16 12:49:56 UTC
Yeah, you're right. GnuTLS does have this requirement too. From https://www.gnutls.org/manual/html_node/Core-TLS-API.html#gnutls_005frecord_005fsend-1 : 

        If the EINTR is returned by the internal push function then
        GNUTLS_E_INTERRUPTED will be returned. If GNUTLS_E_INTERRUPTED or
        GNUTLS_E_AGAIN is returned, you must call this function again, with
        the exact same parameters; alternatively you could provide a NULL
        pointer for data, and 0 for size. cf. gnutls_record_get_direction() . 

But still, we need to make sure that this API requirement is clearly docuumented in all the higher-level APIs which it affects. Not just fix *some* users of those APIs.
Comment 6 Ignacio Casal Quinteiro (nacho) 2017-11-20 09:53:50 UTC
> But still, we need to make sure that this API requirement is clearly
> docuumented in all the higher-level APIs which it affects. Not just fix
> *some* users of those APIs.

Which apis would you suggest to document?
Comment 7 David Woodhouse 2017-11-20 10:45:42 UTC
Any function which, transitively, has this requirement. We know that SSL_write() and gnutls_record_send() have it. But the code you're patching here isn't calling SSL_write() or gnutls_record_send() directly, so where is this requirement documented for the function it *is* calling?
Comment 8 Ignacio Casal Quinteiro (nacho) 2017-11-20 11:43:52 UTC
(In reply to David Woodhouse from comment #7)
> Any function which, transitively, has this requirement. We know that
> SSL_write() and gnutls_record_send() have it. But the code you're patching
> here isn't calling SSL_write() or gnutls_record_send() directly, so where is
> this requirement documented for the function it *is* calling?

I guess we will have to document it on the glib section for TLS support.
Comment 9 David Woodhouse 2017-11-20 13:59:12 UTC
I don't think that's good enough. We need it in the documentation for each function that is affected. So g_pollable_output_stream_write_nonblocking(), for example:
https://developer.gnome.org/gio/stable/GPollableOutputStream.html#g-pollable-output-stream-write-nonblocking

... would need a note saying that *if* the underlying transport happens to be {D,}TLS, and it returns G_IO_ERROR_WOULD_BLOCK, then the next call must have precisely the same data+length.

Note that the caller of g_pollable_output_stream_write_nonblocking() might not always *know* what the underlying stream is. That's kind of the point in class abstractions...

As I said, *every* function which is so affected, wants to be documented.
Comment 10 Ignacio Casal Quinteiro (nacho) 2018-01-24 10:24:52 UTC
Created attachment 367362 [details] [review]
websocket-connection: do not send new frames until the previous is not successfully sent

If the sending of a frame fails with G_IO_ERROR_WOULD_BLOCK,
we must send it again before sending more urgent ones.
This change is relevant in case a SSL connection is being used
because SSL expects the same message to be resent
Comment 11 Ignacio Casal Quinteiro (nacho) 2018-01-24 10:25:40 UTC
For now I just updated the patch to cope with the last changes that happened in master
Comment 12 Ignacio Casal Quinteiro (nacho) 2018-02-01 08:51:54 UTC
Since Dan said the patch looked fine on a previous comment and since the patch I made for glib
for the documentation went in, I am just going ahead and pushing this patch.

Attachment 367362 [details] pushed as e7c827d - websocket-connection: do not send new frames until the previous is not successfully sent