Created attachment 284940 [details] [review]
tlsoutputstream: Use thread-safe GWeakRef

Created attachment 284941 [details] [review]
tlsinputstream: Use thread-safe GWeakRef

Created attachment 284942 [details] [review]
gnutls: Closing an already-closed connection should return TRUE

See the implementation of g_io_stream_close()

Created attachment 284943 [details] [review]
tlsoutputstream: Implement close and close_finish

This allows the application to do a one-way close, to stop sending, but
keep on receiving until the other side is done too.

Created attachment 284952 [details] [review]
tlsoutputstream: Use thread-safe GWeakRef

Also need to set it to NULL in dispose before clearing it
because the parent dispose function tries to call the
close() function.

Created attachment 284953 [details] [review]
tlsinputstream: Use thread-safe GWeakRef

(In reply to comment #0)
> And the old weak ref
> stuff was not thread safe, so I replace it with the new GWeakRef.

We need to clarify the extent to which an iostream and its input/output streams can be used simultaneously from separate threads. Because the only way the weak pointer stuff could be a problem would be if you unreffed the GTlsConnection from one thread while doing a read/write on a child stream from another thread. While *technically* not violating the "you can only use an object from a single thread" rule, this seems pretty broken.

Comment on attachment 284952 [details] [review]
tlsoutputstream: Use thread-safe GWeakRef

seems right, if we do want to guarantee this behavior. Maybe it's even an improvement, clarity-wise, even if we don't care about the thread-safety

Comment on attachment 284943 [details] [review]
tlsoutputstream: Implement close and close_finish

>Subject: [PATCH] tlsoutputstream: Implement close and close_finish
>
>This allows the application to do a one-way close, to stop sending, but
>keep on receiving until the other side is done too.

But this just calls g_tls_connection_close(), so it seems like it's not really one-way...?

>+/* We do async close as synchronous-in-a-thread

Can't we just leave close_async() unimplemented and let the default GOutputStream implementation do synchronous-in-a-thread for us?

(Yes, that same question applies to the GTlsConnection case too... hm...)

The reason to implement it separately on the output stream is that I want to be able to tell the remote side that nothing else will be sent, but I want to wait until it is done sending, not drop the end of what it is sending. And calling g_io_stream_close has the side effect of closing the socket and making the input stream unusable. If you read the GTlsConnectionGnutls code, it actually does only a one way close on the underlying GnuTls connection.

As for sync vs async, I assumed the GTlsConnectionGnutls code was right. 

The multithreading is created by the implicit handshake code.. I create a source for the input or output stream then I do a non blocking read or write which returns G_IO_WOULD_BLOCK (but triggers an implicit handshake in the background) , followed by unreferencing the main iostream, but not the input stream, and it crashes because the GTls code itself closes the last ref on the GTlsConnection, the app then waits for an error from the GSource to drop it's ref on the input or output stream.. So the app is purely single threaded, even if the use case is a bit convoluted, it's basically a splice where the splice code itself by stopping when the main object is destroyed.

(In reply to comment #10)
> And calling
> g_io_stream_close has the side effect of closing the socket and making the
> input stream unusable.

But the code that closes the socket is in g_tls_connection_close(), which you're still calling, and so the socket should end up getting closed anyway. It doesn't seem like there should be any difference between closing the iostream, and closing just the output stream with your patch (other than whether the "closed" property on the GTlsConnection ends up TRUE or FALSE).

> As for sync vs async, I assumed the GTlsConnectionGnutls code was right. 

Yeah, it works, I'm just thinking now that it might not have been necessary... (This doesn't have to be addressed now, just something that occurred to me.)

> The multithreading is created by the implicit handshake code

Ah, right. So yeah, we do need the thread-safety.

Created attachment 285580 [details] [review]
tlsoutputstream: Implement close() and close_async()

(In reply to comment #11)
> But the code that closes the socket is in g_tls_connection_close(), which
> you're still calling, and so the socket should end up getting closed anyway. It
> doesn't seem like there should be any difference between closing the iostream,
> and closing just the output stream with your patch (other than whether the
> "closed" property on the GTlsConnection ends up TRUE or FALSE).

That's a good point, I just checked that gnutls_bye() was called with GNUTLS_SHUT_WR, but it does close the underlying socket. I updated the patch to call g_output_stream_close() only in that case. And call g_io_stream_close() again in all cases, as this will return TRUE immediately for us if the underlying iostream was already closed.

Comment on attachment 284942 [details] [review]
gnutls: Closing an already-closed connection should return TRUE

Committed

commit 6eef612d619629be068e047f8f5931d3be09933d
Author: Olivier Crête <olivier.crete@collabora.com>
Date:   Sun Aug 31 12:17:59 2014 -0400

    gnutls: Closing an already-closed connection should return TRUE
    
    See the implementation of g_io_stream_close()
    
    https://bugzilla.gnome.org/show_bug.cgi?id=735754

Created attachment 286943 [details] [review]
tlsoutputstream: Implement close() and close_async()

This allows the application to do a one-way close, to stop sending, but
keep on receiving until the other side is done too.

I added a unit test, and it's also quite a bit more complex now, because we have to return the right thing from claim_op()/etc.

This patch is on top of the one from bug #736809

Review of attachment 286943 [details] [review]:

::: tls/gnutls/gtlsconnection-gnutls.c
@@ +140,2 @@
   gboolean closing, closed;
+  gboolean write_closing, write_closed;

A comment describing what these booleans mean would be useful.

@@ +512,3 @@
   G_TLS_CONNECTION_GNUTLS_OP_WRITE,
+  G_TLS_CONNECTION_GNUTLS_OP_CLOSE_WRITE,
+  G_TLS_CONNECTION_GNUTLS_OP_CLOSE

And one describing what OP_CLOSE_WRITE is.

It might make sense to introduce OP_CLOSE_WRITE in a separate commit, where it’s initially just an alias for OP_CLOSE. Then a follow-up commit would change its behaviour, and be a lot easier to review.

@@ +531,3 @@
     {
       g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_CLOSED,
+			   _("Connection is closed1"));

This change is unnecessary.

@@ +541,3 @@
+    {
+      g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_CLOSED,
+			   _("Connection is closed2"));

Unnecessary ‘2’.

@@ +1580,3 @@
+					gboolean       both_directions,
+					GCancellable  *cancellable,
+					GError       **error)

I think it would make sense to split the refactoring which introduces close_internal() out into a separate patch.

Also, @both_directions should be an enum for readability.

@@ +1590,3 @@
+    op = G_TLS_CONNECTION_GNUTLS_OP_CLOSE;
+  else
+    op = G_TLS_CONNECTION_GNUTLS_OP_CLOSE_WRITE;

A comment here to explain the process of shutting down reads and writes, or writes only, and how the latter can be upgraded to the former, would be useful.

@@ +1598,3 @@
     {
+      success = g_io_stream_close (gnutls->priv->base_io_stream,
+				   cancellable, error);

Again, this g_io_stream_close() call seems like something which should be in a separate patch. Presumably it’s to handle the case (in current git master) where
 1. g_tls_connection_gnutls_close() is called
 2. The gnutls_bye() call fails
 3. priv->closed is set to TRUE
 4. g_tls_connection_gnutls_close() returns FALSE
 5. g_tls_connection_gnutls_close() is called again and returns immediately because priv->closed is set
 6. g_io_stream_close() is never called

Is that right?

::: tls/tests/connection.c
@@ +1471,3 @@
+
+  read_test_data_async (test);
+  g_main_loop_run (test->loop);

Could add a comment here to make it more prominent that reading data should still work.

@@ +1538,3 @@
               setup_connection, test_async_implicit_handshake, teardown_connection);
+  g_test_add ("/tls/connection/output-stream-close", TestConnection, NULL,
+              setup_connection, test_output_stream_close, teardown_connection);

It would be really good to have a test of this with an implicit handshake too.

Also, with this patch applied, Helgrind detects data races in closing the base I/O stream. These may have existed before. It’s not safe to call g_io_stream_close() on the base I/O stream from within close_thread().

Thread #1 is the rightful owner of the underlying GIOStream (which comes from libnice). Thread #7 is the async worker thread for closing the TLS connection.

==18076== Possible data race during read of size 1 at 0x60BDBD0 by thread #1
==18076== Locks held: none
==18076==    at 0x50D3DAF: g_io_stream_close (giostream.c:389)
==18076==    by 0x4C37A88: streams_removed_cb (iostream.c:358)
==18076==    by 0x54346E1: g_cclosure_marshal_VOID__POINTER (gmarshal.c:1198)
==18076==    by 0x542FDCC: g_closure_invoke (gclosure.c:768)
==18076==    by 0x544DD63: signal_emit_unlocked_R (gsignal.c:3551)
==18076==    by 0x544BE38: g_signal_emitv (gsignal.c:3045)
==18076==    by 0x4C1DD26: agent_unlock_and_emit (agent.c:203)
==18076==    by 0x4C220C8: nice_agent_remove_stream (agent.c:2492)
…
==18076==    by 0x412405: main (main.c:774)
==18076== 
==18076== This conflicts with a previous write of size 1 by thread #7
==18076== Locks held: none
==18076==    at 0x50D3BDE: g_io_stream_set_pending (giostream.c:292)
==18076==    by 0x50D3DD5: g_io_stream_close (giostream.c:392)
==18076==    by 0xCFED742: g_tls_connection_gnutls_close_internal (gtlsconnection-gnutls.c:1695)
==18076==    by 0xCFED8FC: g_tls_connection_gnutls_close (gtlsconnection-gnutls.c:1740)
==18076==    by 0xCFED93D: close_thread (gtlsconnection-gnutls.c:1756)
==18076==    by 0x51033C5: g_task_thread_pool_thread (gtask.c:1229)
==18076==    by 0x56F97D7: g_thread_pool_thread_proxy (gthreadpool.c:340)
==18076==    by 0x56F90D9: g_thread_proxy (gthread.c:764)

(In reply to comment #16)
> Also, with this patch applied, Helgrind detects data races in closing the base
> I/O stream. These may have existed before. It’s not safe to call
> g_io_stream_close() on the base I/O stream from within close_thread().
> 
> Thread #1 is the rightful owner of the underlying GIOStream (which comes from
> libnice). Thread #7 is the async worker thread for closing the TLS connection.

Actually, I think the GTlsConnection is the rightful owner of the GIOStream because the application gave the libnice GIOStream to the GTlsConnection when creating it. So the streams_removed_cb is wrong to call g_io_stream_close() as libnice doesn't "own" it GIOStream anymore. I think streams_removed in libnice should do something internal instead (set some flag or something). So that's a libnice bug.

(In reply to comment #15)
> Review of attachment 286943 [details] [review]:
> 
> ::: tls/gnutls/gtlsconnection-gnutls.c
> @@ +1598,3 @@
>      {
> +      success = g_io_stream_close (gnutls->priv->base_io_stream,
> +                   cancellable, error);
> 
> Again, this g_io_stream_close() call seems like something which should be in a
> separate patch. Presumably it’s to handle the case (in current git master)
> where
>  1. g_tls_connection_gnutls_close() is called
>  2. The gnutls_bye() call fails
>  3. priv->closed is set to TRUE
>  4. g_tls_connection_gnutls_close() returns FALSE
>  5. g_tls_connection_gnutls_close() is called again and returns immediately
> because priv->closed is set
>  6. g_io_stream_close() is never called
> 
> Is that right?

Actually, my use-case was to return errors from the underlying GIOStream if there was one. But I think this will never get called because claim_op() will return FALSE, which is wrong. And actually, this whole code path may never be called because the GIOStream base class will set a closed flag and not call the subclass again. Unless two threads call into close at the same time, which is illegal. But that gets more complex if one thread does g_output_stream_close() and one does g_io_stream_close().
 
> @@ +1538,3 @@
>                setup_connection, test_async_implicit_handshake,
> teardown_connection);
> +  g_test_add ("/tls/connection/output-stream-close", TestConnection, NULL,
> +              setup_connection, test_output_stream_close,
> teardown_connection);
> 
> It would be really good to have a test of this with an implicit handshake too.

I tried, but then it closes it before the handshake happens. Testing "while" the implicit handshake it going on isn't really possible right now I think.

Created attachment 286993 [details] [review]
tlsoutputstream: Implement close() and close_async()

Updated for some of Philip's comments, I ignored the ones about patch splitting as I'm lazy. I'm not sure I have all of the correct use-cases in mind, but I'm thinking that maybe this could be made more simple by only having a flag for if gnutls_bye was already called, and in other cases just calling g_io_stream_close()/g_output_stream_close() on the underlying stream. But I'm not certain I fully understand what exactly the claim_op()/yield_op() operations are designed for.

(In reply to comment #17)
> Actually, I think the GTlsConnection is the rightful owner of the GIOStream
> because the application gave the libnice GIOStream to the GTlsConnection when
> creating it. So the streams_removed_cb is wrong to call g_io_stream_close() as
> libnice doesn't "own" it GIOStream anymore. I think streams_removed in libnice
> should do something internal instead (set some flag or something). So that's a
> libnice bug.

I think that’s wrong*, and we should be thinking of it in terms of which GMainContext each GIOStream belongs to. The base GIOStream (from libnice) is attached to a GMainContext before the TLS connection is constructed around it, so I think the TLS connection has to ensure that the base GIOStream methods are only called when that GMainContext is owned by the current thread, which is not the case here.

Note that the documentation for g_tls_connection_set_require_close_notify() explicitly says that it’s allowed for application code (i.e. libnice) to close the underlying base GIOStream to achieve an unclean shutdown.

From that point of view, I’m not sure the existing GTlsConnectionGnutls implementation (without the patch applied) is thread safe for the base GIOStream, since e.g. g_tls_connection_gnutls_check() is called synchronously by GTlsInputStreamGnutls and uses base_istream methods, but so does g_tls_connection_gnutls_pull_func() and that can be called synchronously or in a worker thread (e.g. from g_tls_connection_gnutls_read() vs. handshake_thread()).

*But I am nowhere near an expert on this code and would rather hear what Dan thinks.

(In reply to comment #18)
> Actually, my use-case was to return errors from the underlying GIOStream if
> there was one.

Perhaps an approach using a close_error (like handshake_error) would work better then?

> But I think this will never get called because claim_op() will
> return FALSE, which is wrong.

Oops, yes.

> > It would be really good to have a test of this with an implicit handshake too.
> 
> I tried, but then it closes it before the handshake happens. Testing "while"
> the implicit handshake it going on isn't really possible right now I think.

Couldn’t you iterate the GMainContext in the test a certain number of times until it’s part-way through the handshake? I guess that would be a bit flaky.

I suppose the real solution would be to use an underlying GDummyIOStream which allows the test harness to get a callback after each packet, or something, then it could reliably call GTlsConnection methods at precise stages in the handshaking process. Isn’t testing fun?

(In reply to comment #19)
> Created an attachment (id=286993) [details] [review]
> tlsoutputstream: Implement close() and close_async()
> 
> Updated for some of Philip's comments, I ignored the ones about patch splitting
> as I'm lazy.

10 minutes with `git rebase` and `git add -i`!

> But I'm
> not certain I fully understand what exactly the claim_op()/yield_op()
> operations are designed for.

afaik, they are basically the state machine implementation for the GTlsConnection. claim_op() checks whether a given operation can be started, and updates the state machine on success. yield_op() finishes an operation. Between them they handle blocking on pending operations and handling cancellation correctly.

(In reply to comment #20)
> (In reply to comment #17)
> > Actually, I think the GTlsConnection is the rightful owner of the GIOStream
> > because the application gave the libnice GIOStream to the GTlsConnection when
> > creating it. So the streams_removed_cb is wrong to call g_io_stream_close() as
> > libnice doesn't "own" it GIOStream anymore. I think streams_removed in libnice
> > should do something internal instead (set some flag or something). So that's a
> > libnice bug.
> 
> I think that’s wrong*, and we should be thinking of it in terms of which
> GMainContext each GIOStream belongs to. The base GIOStream (from libnice) is
> attached to a GMainContext before the TLS connection is constructed around it,
> so I think the TLS connection has to ensure that the base GIOStream methods are
> only called when that GMainContext is owned by the current thread, which is not
> the case here.

Streams are not associated with particular threads/GMainContexts; it's only the individual operations on them that are. You should be able to write to a stream from one thread, and then close it from a different thread, as long as the write operation is completely finished before the close operation starts.

(This is not documented anywhere, and probably needs to be, since it's not obvious. But gio regularly assumes that it can run stream operations in random other threads without telling you. g_output_stream_splice_async() is another place where you're likely to hit this.)

> Note that the documentation for g_tls_connection_set_require_close_notify()
> explicitly says that it’s allowed for application code (i.e. libnice) to close
> the underlying base GIOStream to achieve an unclean shutdown.

Though it's implied that you can only do this if there is no other operation in progress, so there shouldn't be any problem. (That's supposed to be enforced by the API, but bug 707912.)

(In reply to comment #17)
> Actually, I think the GTlsConnection is the rightful owner of the GIOStream
> because the application gave the libnice GIOStream to the GTlsConnection when
> creating it. So the streams_removed_cb is wrong to call g_io_stream_close() as
> libnice doesn't "own" it GIOStream anymore. I think streams_removed in libnice
> should do something internal instead (set some flag or something). So that's a
> libnice bug.

Hmm, yes, so after what Dan said and some further consideration, it looks like libnice has to change, but I can’t think of a decent way of changing it. Our application code was incorrectly calling nice_agent_remove_stream() before the g_io_stream_close_async() on that stream had completed, which was causing the problem. I should have seen that before. :-\

For the moment, I’ve pushed a documentation change to libnice (7de8206de2b86d4e092f5e05405a907337fbec4d) which documents this. We should come up with some change to the behaviour of streams_removed_cb() to fix it properly though.

Created attachment 287176 [details] [review]
giostream: Fix some typos in the GIOStream documentation

This doesn’t change the meaning of the documentation.

Created attachment 287177 [details] [review]
gio: Document thread safety of the streams API

Specifically, GIOStream and the TLS connection streams.

Created attachment 287179 [details] [review]
gnutls: Add OP_CLOSE_WRITE and _READ operations to GTlsConnectionGnutls

This introduces no functional changes, and is a first step to supporting
half-duplex close().

Created attachment 287180 [details] [review]
gnutls: Expose g_tls_connection_close() internally

This will shortly be used to implement half-duplex close().

Created attachment 287181 [details] [review]
gnutls: Implement half-duplex close for GTlsConnectionGnutls

Support closing the read or write substreams of a GTlsConnectionGnutls
independently. This includes the core support for half-duplex close, but
not the implementations in GTls[Input|Output]StreamGnutls.

Based on a patch by Olivier Crête <olivier.crete@collabora.com>.

Created attachment 287182 [details] [review]
gnutls: Implement half-duplex close in GTlsOutputStreamGnutls

Including unit tests.

Based on a patch by Olivier Crête <olivier.crete@collabora.com>.

Created attachment 287183 [details] [review]
gnutls: Implement half-duplex close in GTlsInputStreamGnutls

Comment on attachment 286993 [details] [review]
tlsoutputstream: Implement close() and close_async()

So I’ve split the patch up, and added support for closing the read side of the connection too, which improves the symmetry and hopefully makes it all a little clearer.

The other main changes are in g_tls_connection_gnutls_close_internal() to try closing the underlying streams even if gnutls_bye() fails, since we only get one chance due to the ->closed flag in the GIOStream parent implementation.

I didn’t get a chance to write any unit tests additional to what Olivier has already written. I’ll try and do that on Monday.

Created attachment 288728 [details] [review]
gnutls: Implement half-duplex close for GTlsConnectionGnutls

Support closing the read or write substreams of a GTlsConnectionGnutls
independently. This includes the core support for half-duplex close, but
not the implementations in GTls[Input|Output]StreamGnutls.

Based on a patch by Olivier Crête <olivier.crete@collabora.com>.

Created attachment 288729 [details] [review]
gnutls: Implement half-duplex close in GTlsInputStreamGnutls

(In reply to comment #32)
> I didn’t get a chance to write any unit tests additional to what Olivier has
> already written. I’ll try and do that on Monday.

Unit tests for GTlsInputStreamGnutls closing have been added in attachment #288729 [details], and a minor bug fixed in attachment #288728 [details].

“I’ll try and do that on Monday.” — utter lies. :-(

Comment on attachment 287176 [details] [review]
giostream: Fix some typos in the GIOStream documentation

Committed the trivial documentation typo fixes.

Attachment 287176 [details] pushed as 21809c8 - giostream: Fix some typos in the GIOStream documentation

Comment on attachment 287177 [details] [review]
gio: Document thread safety of the streams API

>+ * Operations on #GIOStreams (and by extension their substreams too) may only be
>+ * run in different threads if all previous operations have completed. So it is
>+ * valid for an application to write to a #GIOStream in one thread, wait for the
>+ * write to complete, then close the #GIOStream from another thread. But it is
>+ * not valid to write to a #GOutputStream substream from one thread concurrently
>+ * with reading from the #GInputStream substream in another.

Actually, allowing exactly that use case is the reason GTlsConnection is so complicated. (The claim_op/yield_op stuff.)

So, the rule is supposed to be: you can read from the input stream and write to the output stream at the same time (either in separate threads, or as simultaneous asynchronous operations in a single thread), but you can't start any GIOStream operation while there is a GInputStream or GOutputStream operation in progress, and you can't start any GInputStream or GOutputStream operation while there is a GIOStream operation in progress.

(I'm not convinced that all existing GIOStreams support these semantics, but GTlsConnection does, and there are tests for it: /tls/connection/simultaneous-async and /tls/connection/simultaneous-sync.)

>+ * as #GBufferedInputStream or #GTlsConnection. With such wrapper APIs,
>+ * application code may only run operations on the base (wrapped) stream when
>+ * the wrapper stream is idle.

Although the semantics of doing so are not necessarily well-defined (other than "not crashing".) Eg, reading off a GFilterInputStream may or may not still succeed after closing the base stream.

Comment on attachment 287179 [details] [review]
gnutls: Add OP_CLOSE_WRITE and _READ operations to GTlsConnectionGnutls

>+      if (op != G_TLS_CONNECTION_GNUTLS_OP_CLOSE_BOTH &&
>+          op != G_TLS_CONNECTION_GNUTLS_OP_CLOSE_READ &&
>+	  op != G_TLS_CONNECTION_GNUTLS_OP_CLOSE_WRITE &&
>           gnutls->priv->need_handshake)

space vs tab issues

Comment on attachment 287180 [details] [review]
gnutls: Expose g_tls_connection_close() internally

why do this rather than just making g_tls_connection_gnutls_close() non-static?

Comment on attachment 288728 [details] [review]
gnutls: Implement half-duplex close for GTlsConnectionGnutls

> gboolean
> g_tls_connection_gnutls_close_internal (GIOStream     *stream,
>+                                        GTlsDirection  direction,

ok, i guess this answers my previous question about why close_internal.

I don't think we really need GTlsDirection. Just use GIOCondition.

>+  /* Close the underlying streams. Do this even if the gnutls_bye() call failed,
>+   * as the parent GIOStream will have set its internal closed flag and hence
>+   * this implementation will never be called again. */

The idea is right, but the implementation is wrong; if gnutls_bye() and g_*_stream_close() both fail, the latter will try to overwrite an already-set GError.

Comment on attachment 284953 [details] [review]
tlsinputstream: Use thread-safe GWeakRef

d045932 tlsinputstream: Use thread-safe GWeakRef
3e7af08 tlsoutputstream: Use thread-safe GWeakRef

a_c-n patches rebased on master, the minor review issues fixed, `make check` passes, and all pushed. I’ll update the documentation patch shortly. Thanks for the reviews!

(In reply to Dan Winship from comment #38)
> Comment on attachment 287179 [details] [review] [review]
> gnutls: Add OP_CLOSE_WRITE and _READ operations to GTlsConnectionGnutls
> 
> >+      if (op != G_TLS_CONNECTION_GNUTLS_OP_CLOSE_BOTH &&
> >+          op != G_TLS_CONNECTION_GNUTLS_OP_CLOSE_READ &&
> >+	  op != G_TLS_CONNECTION_GNUTLS_OP_CLOSE_WRITE &&
> >           gnutls->priv->need_handshake)
> 
> space vs tab issues

Whoops, fixed in the pushed version.

(In reply to Dan Winship from comment #40)
> Comment on attachment 288728 [details] [review] [review]
> gnutls: Implement half-duplex close for GTlsConnectionGnutls
> 
> I don't think we really need GTlsDirection. Just use GIOCondition.

I think it makes the code a fair bit clearer, and avoids questions about ‘what does G_IO_HUP mean in this context?’. It’s not exposed publicly. I’ve pushed these patches with GTlsDirection still there.

> >+  /* Close the underlying streams. Do this even if the gnutls_bye() call failed,
> >+   * as the parent GIOStream will have set its internal closed flag and hence
> >+   * this implementation will never be called again. */
> 
> The idea is right, but the implementation is wrong; if gnutls_bye() and
> g_*_stream_close() both fail, the latter will try to overwrite an
> already-set GError.

Oops. Fixed to return the gnutls_bye() error if it’s set, otherwise to return the g_*_stream_close() error (or success).

Attachment 287179 [details] pushed as dbd2597 - gnutls: Add OP_CLOSE_WRITE and _READ operations to GTlsConnectionGnutls
Attachment 287180 [details] pushed as afde448 - gnutls: Expose g_tls_connection_close() internally
Attachment 287182 [details] pushed as 8d43835 - gnutls: Implement half-duplex close in GTlsOutputStreamGnutls
Attachment 288728 [details] pushed as 334d098 - gnutls: Implement half-duplex close for GTlsConnectionGnutls
Attachment 288729 [details] pushed as 6470a33 - gnutls: Implement half-duplex close in GTlsInputStreamGnutls

Created attachment 318752 [details] [review]
gio: Document thread safety of the streams API

Specifically, GIOStream and the TLS connection streams.

Includes wording adapted from suggestions by Dan Winship
<danw@gnome.org>.

Removed two extraneous paragraphs after review by danw on IRC; pushed.

The following fix has been pushed:
3add5e2 gio: Document thread safety of the streams API

Created attachment 318759 [details] [review]
gio: Document thread safety of the streams API

Specifically, GIOStream and the TLS connection streams.

Includes wording adapted from suggestions by Dan Winship
<danw@gnome.org>.