Created attachment 363800 [details] [review]
websocket-connection: do not send new frames until the previous is not successfully sent

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...

> 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 on attachment 363800 [details] [review]
websocket-connection: do not send new frames until the previous is not successfully sent

seems right... probably?

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.

> 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?

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?

(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.

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.

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

For now I just updated the patch to cope with the last changes that happened in master

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