Re: Review of gnio, round 1

[Alexander Larsson <alexl redhat com>]

On Mon, 2009-04-27 at 11:39 -0400, Ryan Lortie wrote:
> Alexander Larsson wrote:
> > GIOStream.c:
> > ------------
> > This is currently an interface, while the related GInputStream and
> > GOutputStreams are base classes. This isn't necessarily wrong by itself,
> > but looks a bit weird. There are also other reasons (see below) why it
> > would be nice if it was a class, so I think we should change it.
> 
> For all the reasons that you list, I agree.  I'll do this.

Cool.

> > When GIOStream is used for a two-way stream it really represent a single
> > object that has two directions for i/o. As such the individual streams
> > should not be closed by themselves, for instance it makes no sense to
> > close the input stream on a network socket or on a readwrite local file
> > fd. So, we need to add close and close_async operations on GIOStream. We
> > also want to add an is_closed() on the GIOStream.
> 
> Not sure I agree.  See shutdown() syscall.  The fact that this call 
> exists means that the designers of [unix or tcp or whatever] went out of 
> their way because they disagreed with you.

Well, yeah, I guess I chose a poor example. Replace with e.g. a
read/write local file fd. My point is that we really need a close
operation that encompasses the whole object, not the individual streams.

> > We also need to define what closing a substream means. This is a bit
> > tricky, and may actually be implementation specific, as closing one
> > direction may make sense (for instance using shutdown() on a TCP socket.
> > I think in most cases we should just chain to the default
> > GInput/OutputStream close methods, which will set the "closed" state and
> > which will cause all operations to fail on that stream, then implement
> > the real close in GIOStream, which should also close the individual
> > streams to allow special handling there.
> 
> OK.  So now I see that you propose to have a close() on the toplevel 
> stream but also the ability to individually close only one of the 
> substreams?  Is that an accurate assessment?

Yes, and furthermore, once the main object is closed the individual
streams should act as if they were closed. I.e.
g_input_stream_is_closed() returns TRUE, and all operations fail with
G_IO_ERROR_CLOSED. The easiest way to achieve this is to actually call
the close operation on the separate streams, as the GInput/OutputStream
base implementation will set closed to TRUE. Although you probably need
to ensure g_io_stream_is_closed() returns TRUE first so any custom close
implementation of the streams (like the shutdown thing) can detect this
case.


> Going forward from this point, you argue that there should be a massive 
> reduction in the number of classes.  I think that your decision-making 
> here is influenced by the fact that we're working in C.  I tried to 
> think out a logical class structure that I would do as if I had a 
> language like Vala to do the dirty work for me -- that is, I favoured 
> subclassing where it makes sense for conceptual clarity rather than in 
> order to avoid an explosion of classes.

That is not quite correct. I don't mind having classes if it e.g. means
more typing for the implementation. In fact, this is pretty obvious if
you look at gio itself which is pretty well factored wrt classes and
interfaces. However, what I think is bad about the many classes is that
they increase the complexity (or at least the percieved complexity) of
the API. There is a lot more classes to understand when e.g. reading the
docs, and its a lot harder to keep the class hierarchies and
dependencies in your head. This is especially bad when many classes
don't add any advantages to the user. 

Its also a problem because it means its a pain in the ass to extend the
API. For instance, with the minor change i'm about to do in the socket
construction case we will already support SCTP sockets. However, we need
a full suite of useless subclasses in order for it to match the tcp
case.

> The result, of course, was an explosion of classes.  I don't think that 
> that is a bad thing, per se.
> 
> > GSocketClient, GTcpClient, GUnixClient
> > ======================================
> > The base class is basically a useless temporary class you have to create
> > such that you can call the connect() or connect_async() call on it. The
> > subclasses are needed in order to create GSocketConnections of the right
> > subclass, and they have a few helper function that lets you connect
> > without having to first create a socket address.
> > 
> > If we drop the GSocketConnection subclasses the type specific creation
> > is not necessary. For the helper functions, I think we should drop all
> > but the hostname one, as this is the only really common one and the
> > others can easily do with the generic calls.
> > 
> > So, I propose we should move the connect calls to GSocketConnection
> > functions:
> > 
> > GSocketConnection *g_socket_connection_connect_to (GSocketConnectable
> > *connectable,
> > 		  				   GCancellable        *cancellable,
> >                     				   GError             **error);
> > GSocketConnection *g_socket_connection_connect_to_host (const gchar
> > *host,
> >                                                         const gchar
> > *default_port,
> > 							GCancellable        *cancellable,
> >   							GError             **error);
> > Plus g_socket_connection_connect_to_async, and
> > g_socket_connection_connect_to_host_async variants.
> > 
> > This should replace all the g*client.[ch] code.
> 
> We had a lot of discussions about this (in #vala, of all places) back in 
> the day.  We have the client code for 3 reasons:
> 
>   - the reason you cite about creating the right type of connection

This only happens if we enforce different type for connections though.

>   - in order to be able to specify connection parameters (like the local
>     address to connect from, or socket options) without having gigantic
>     connect() calls with 20 arguments.  also, if we decide to add more
>     options or flags later (by user request) then it's just a matter of
>     adding new functions -- not changing existing api.

Are there really that many things you can specify for the unconnected
socket? Especially in this easy to use helper class/function. Although
it is kind of nice to have the async connect functionallity availible to
use even if you're not using the helper classes.

Maybe we could move the code for async accept and connect to GSocket?
You wouldn't have to use it, but you could, and the GSocketConnection
related code would then use that.

>   - there's no pattern for async IO without a gobject on which to perform
>     the IO (ie: the source_object for the async result and callback).

This is a bit of a bummer, yeah.

> If we want to solve this properly, then I propose that we come up with 
> two new generic interfaces:
> 
>   - asynchronous object creation interface
> 
>   - potentially failing object creation interface
> 
> The async interface would essentially look like a complete() and 
> complete_async() call that you do right after g_object_new().  The 
> object isn't considered to be fully constructed until that returns.

Interface as in a GInterface, or as an API pattern that all
implementations would follow?

> The potentially-failing object creation interface would be a 
> complete(GError *err) call that you do after _new().  If it fails, then 
> the object must be immediately destroyed and not used.
> 
> We might even add some g_object_new() type API to GIO 
> (g_object_async_new ?!) that did the checking and async handling.
> 
> If we developed those as proper interfaces that other people could use 
> then I'd be comfortable with setting a new precedent for using them in 
> GNIO, but that still solves only 1 of the above 3 problems.
> 
> > GSocketConnection, GTcpConnection, GUnixConnection
> > ==================================================
> > 
> > GTcpConnection is empty, so just remove.
> 
> Again, this is just WIP.  The intention is to land things like TCP 
> connection options here (like CORK, etc).

I dunno how important such things are. We shouldn't let minor features
that will not be used by the vast majority complicate the API for
everyone. At some level if you need a special feature you can just get
the socket object from the socketconnection and use the lowlevel apis. 

> > I think we should add
> > g_socket_connection_send_control_message(connection, scm) to
> > GSocketConnection. This is a general function that is useful for other
> > socket types too. Then we push all the fd specific stuff to
> > GUnixFdMessage. This makes passing fds a few more lines of code, but
> > this is imho not a huge problem, as this is not a common operation.
> 
> This wouldn't be terrible, although I have a preference to the way it's 
> currently done.  It's worth noting that send_fd() is actually slightly 
> evil in the sense that it sends a single zero byte (you can't send 
> control messages without sending at least one byte).  This "sending a 
> zero byte" seems to be a fairly well-established thing to do, but it's 
> certainly not the most generic way that this call could work.

Yes, the zero byte convention (although if this is done or not depends
on the protocol in use). Does makes something like this a bit trickier,
as you don't have a good place to encode this convention. You don't want
to do it in general, because its not needed for all kinds of messages.

> > GSocketListener, GSocketService, GTcpListener, GUnixListener
> > ============================================================
> > 
> > A socket listener is basically a wrapper around a GSocket that is bound
> > to a specified address and implements the async accept functionallity.
> > The subclasses has constructors that let you type less. Also, the tcp
> > listener has some magic to pick the right kind of socket (ipv4 or ipv6).
> > 
> > Then you add all the listeners to a GSocketService that lets you listen
> > to multiple sockets for incomming connections.
> > 
> > I'm not sure that the listener object is really needed. Wouldn't it make
> > more sense to just add socketaddress objects to the socket service
> > instead. The listener is one-to-one with a both the socket and the
> > socket-address anyway, so I don't see how it buys us something.
> I find your argument here fairly convincing.  The only real reason to 
> have the listener in this particular use case is to create the correct 
> type of connection object.
> 
> In other use cases, though, the listener object itself -is- directly 
> useful.  Some people might want the accept_async() style rather than the 
> signal callback style.  Vala's 'yields' functions and davidz's fibres 
> would work particularly well with this.  That's the use case for 
> separate listener objects.

Aell. For things that want to implement socket services themselves we
could put the accept_async implementation in GSocket (as mentioned
above), then apps could use that (and the GSocketService would use it
too).

> > Furthermore, its actually a problem in the ipv4 vs ipv6 magic case. The
> > current tcplistener code first tries to do an ipv6 socket and only if
> > that fails it tries an ipv4 socket. This makes sense on linux, were an
> > ipv6 socket also can accept ipv4 connections. However, this is not true
> > on many other unixes, where you need two sockets to handle both ipv4 and
> > ipv6. So in this case the listener object actually gets in the way, as
> > we'd need to create two listener objects to handle this (or make the
> > listener have two sockets).
> 
> The 4-over-6 functionality (and even the setsockopt to disable it) is 
> specified in some RFC somewhere.
>
> 
> http://www.faqs.org/rfcs/rfc3493.html, section 5.3.

Well, spec or not, but as danw points out, and as I've seen mentioned
many times on the net, in practice only linux does this.

There is a further complexity, mentioned in:
http://people.redhat.com/drepper/userapi-ipv6.html
I.e. it turns out that linux doesn't allow multiple sockets to bind to
the same port, so on linux you *have* to use an ipv6 socket with
4-over-6, while on other OS:es this doesn't work...

> > Other random stuff
> > ==================
> > 
> > The unix socket code really should support linux-style abstract
> > socketnames. Ideally in some way that makes it easy to fall back if this
> > is not supported.
> 
> Sure.
> 
> I also think that we could use some sort of 'localsocket' abstraction 
> that does unix sockets on unix and maybe some sort of named pipe stuff 
> on windows.  We should then encourage people to use this instead of unix 
> sockets (unless they really need unix sockets for fd passing or 
> something like that that won't work on windows anyway).

I remember reading recently that there was all kinds of issues with
local pipes on win32, but I don't remember any details. It was on some
gnome list, maybe even this.