Created attachment 174019 [details] [review]
Add pollable input/output streams

When interfacing with APIs that expect unix-style async I/O, it is
useful to be able to tell in advance whether a read/write is going to
block. This adds new interfaces GPollableInputStream and
GPollableOutputStream that can be implemented by a GInputStream or
GOutputStream to add _is_readable/_is_writable, _create_source, and
_read_nonblocking/_write_nonblocking methods.

Also, implement for GUnixInput/OutputStream and
GSocketInput/OutputStream

Docs for the new APIs:
http://people.gnome.org/~danw/tls-docs/gio/GPollableIOStream.html
http://people.gnome.org/~danw/tls-docs/gio/GPollableInputStream.html
http://people.gnome.org/~danw/tls-docs/gio/GPollableOutputStream.html

The name is odd.  meh.

I wouldn't mind seeing some way to create a GSource from the GPollableIOStream with a set of IO conditions.

The idea of using an interface as a marker for some sort of guarantee about the dynamic type that will be returned from a virtual function with a given static type is inelegant since it requires downcasting.  I'd almost prefer introduction of a new set of virtual methods (that possibly overload the name of the generic ones) with the promised types given in a static way.  To be strictly correct these new virtual methods would require new vtable entries too, but maybe that's going too far.

I wonder if the more convenient thing to provide to people who want this sort of interface is not just a generic GSocket-like interface (with read/write on the main object).  The input/output stream stuff is useful as separate objects mostly because you can use filter streams, but that's not important here.

Returning 0 instead of an error might actually be a nicer API to use.  I hate having to match error codes in order to determine what happened.

nitpick: convention dictates _real_ instead of _default_.

Also: why interface instead of abstract subclass?

This looks good to me. One issue though: There are some stream types where the "is pollable" check is not static, but rather runtime dynamic. For instance, a unix fd may support nonblocking/polling or not depending on what type of object it refers to. This means encoding it in the typesystem isn't ideal.

GSeekable has the same kind of problem, which is why it also has:
 gboolean g_seekable_can_seek     (GSeekable     *seekable);

I think these interfaces need something similar.

(In reply to comment #3)
> I wouldn't mind seeing some way to create a GSource from the GPollableIOStream
> with a set of IO conditions.
...
> I wonder if the more convenient thing to provide to people who want this sort
> of interface is not just a generic GSocket-like interface (with read/write on
> the main object).

My original implementation had just "GPollable", with g_pollable_check() and g_pollable_create_source() pretty much exactly parallel to the GSocket functions. (Originally I also had g_pollable_wait(), but then I realized I wasn't ever using it, because there's no point, and in fact, there's no point to g_socket_wait() either, because g_socket_send/receive do it for you automatically, which goes back to what you were saying this weekend about those APIs not working the way you'd originally intended any more.)

Anyway, I changed it because it seemed slightly odd to me (since GIOStream doesn't do anything and all the real operations are on GInputStream and GOutputStream, so it seemed like we ought to behave parallelly here) and because it seemed possible that someone might want to have a pollable input stream with no corresponding output stream or vice versa.

(It also seemed like it was going to make certain bits of code cleaner, because they'd just be making either a GInputStream call or a GPollableInputStream call on the same stream object (http://git.gnome.org/browse/glib-networking/tree/tls/gnutls/gtlsconnection-gnutls.c?h=tls#n690) instead of having to act on either the GInputStream object or the GPollable object depending on whether or not they want blocking. But it turned out to make other bits of code dirtier, because now they have to act on either the input stream or the output stream depending on what they're doing (http://git.gnome.org/browse/glib-networking/tree/tls/gnutls/gtlsconnection-gnutls.c?h=tls#n593).

But anyway, I could change this back, if we think that the API inconsistency isn't too weird, and we don't mind losing the possibility of pollable-input-stream-without-pollable-output-stream.

> Returning 0 instead of an error might actually be a nicer API to use.  I hate
> having to match error codes in order to determine what happened.

I don't understand what you mean there. (If we returned 0 and no error then you wouldn't have to match error codes, but you wouldn't have any other way to determine what happened either...)

> nitpick: convention dictates _real_ instead of _default_.

even for interfaces?

(In reply to comment #4)
> Also: why interface instead of abstract subclass?

Because in some cases you can take a non-pollable iostream and make a pollable subclass of it. (Eg, not that we have GFilterIOStream, but if we did, it would not be pollable in the general case, and yet you might have some subclass of it that was always pollable.)

(In reply to comment #5)
> GSeekable has the same kind of problem, which is why it also has:
>  gboolean g_seekable_can_seek     (GSeekable     *seekable);
> 
> I think these interfaces need something similar.

ah, yes, good. I was ignoring the problem of, say, a GSocketConnection subclass needing to be able to declare itself non-pollable for some reason.

although in that case (assuming we don't go down the path Ryan suggested) there's no longer any point in GPollableIOStream, since it exists only to provide typesafe guarantees that you can't construct a GTlsConnection from a non-pollable iostream. So we could just ditch that and have only the pollable input/output streams, and GTlsConnection:base-io-stream would become a GIOStream instead of GPollableIOStream, and g_tls_connection_initable_init() would just have to do a sanity check.

Oops.  I forgot to consider the "socket got disconnected" case when I was talking about the return-zero thing.

Are we ever going to have a non-pollable fd that we would use as a stream?  The only case I can think about is normal files and I guess those are just always read/writable...

ryan: I wouldn't want to treat disk files as always readable/writable, as that leads you to believe they are non-blocking, which they are not.

Also, there are other kinds of non-pollable fds, like opened device nodes, etc.

from IRC last night:

<desrt> danw: it also seems that this thing is at a fundamentally lower level than the IOStream stuff
<desrt> mostly because it's clear that you can implement an IO stream on top of a GSocket-type interface with minimal pain but the inverse is not true
<desrt> (i think we've both indepedently tried to go that way)
<desrt> *independently
<danw> yes
<desrt> note, for example: GSocket doesn't implement GIOStream
<desrt> but i would expect it to implement your new interface
<desrt> which it can't do without also implementing GIOStream
<danw> desrt: well, this was specifically an interface for people who are using GIOStreams, so no, I wouldn't expect GSocket to implement it. but yes to the larger point
<desrt> the problem is that there is no clear path to implement the interface for existing IOStreams
<desrt> which is really exactly why it has to be a separate interface
<desrt> so there is clearly a mismatch of ideas here
<danw> well, it's clear how you implement it for GUnixInput/OutputStream, and GTlsInput/OutputStream. And WockyTestStream
<danw> it's not clear how you'd implement it in iostreams that aren't pollable, but that's the point
<desrt> right
<desrt> interfaces that have other interface prerequisites are usually building upon those interfaces rathetr than providing an entirely separate class of functionality
<danw> [citation required]
<desrt> i guess my main point is that GPollableIOStream is really easy to implement
<desrt> and GIOStream is not
<danw> ?
<danw> did you say that backwards?
<desrt> no.
<desrt> imagine you have an fd (which is really what we're talking about here)
<desrt> and for sake of argument, gnutls's stream interface is sufficiently "fd like"
<danw> ok, i see what you mean
<desrt> in that case implementing GPollableIOStream is positively trivial
<desrt> so trivial that i have a hard time imagining why we don't do it in GSocket
<danw> do what in GSocket? implement TLS?
<desrt> implement the 'pollable io interface'
<desrt> ie: generic read/write/poll iface
<danw> yeah, if we're moving away from having separate pollable input/output streams, there's no reason we couldn't
<desrt> and have TLS consume that
<desrt> (and also provide that, naturally)
<desrt> i guess it's a net line of code decrease in just about every way...
<desrt> and also a performance/memory improvement



so, I started implementing this last night, and reimplemented basically all of this patch, but then when I got to implementing TLS on top of it, I realized that if GTlsConnection isn't going to take a GIOStream, then you need the "GPollable" interface to provide a close() method too. And if you want GTlsConnection to *implement* the GPollable interface, then close() needs to be able to return G_IO_ERROR_WOULD_BLOCK and then you need condition_check() and create_source() to have a "closable" condition as well.

also, if we build GTlsConnection directly on top of GSocket rather than GSocketConnection, then we lose the ability to use potentially useful GTcpConnection / GSctpConnection methods. (Though for the moment at least, there are no GTcpConnection methods that are useful for TLS connections, and there is no GSctpConnection at all.)

so I'm leaning a little more towards what i said in comment #7. (GPollableInputStream and GPollableOutputStream, but no GPollableIOStream).

(In reply to comment #10)
> so I'm leaning a little more towards what i said in comment #7.
> (GPollableInputStream and GPollableOutputStream, but no GPollableIOStream).

That's what I ended up doing. Those two interfaces make sense on their own, and continue to make sense when implemented by other existing classes, whereas the "GSocketesque" object (which we never did come up with a good name for) would end up being a wart almost everywhere it appeared, because of pre-existing APIs that should have been based on it but weren't.

Attachment 174019 [details] pushed as c20c2c0 - Add pollable input/output streams