Re: Review of gnio, round 1

[Ryan Lortie <desrt desrt ca>]

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.

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

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

> gunixcredentialsmessage.c:
> --------------------------
> Doesn't seem used, and has parts of gunixfdmessage.c in it

WIP.  Never got around to implementing this. :)


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.

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

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

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


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.

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

>                                          GUnixConnection has helper
> functions for sending and recieving a file descriptor. This is very
> specific to unix sockets, and is something that probably should be
> unix-only (i.e. in the gio-unix-2.0.pc file). As such we'd like to avoid
> having it directly in a shared header. This is sort of hard to solve in
> a way that is as easy to use as g_unix_connection_send_fd(). However, I
> don't think its worth making the rest of the API vastly larger and more
> complex just to get this one call. However, we should still make it
> pretty easy to pass fds.
Also, credentials.


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


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

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

...

...

ok, I'm not really that lazy :)

http://www.faqs.org/rfcs/rfc3493.html, section 5.3.


> I think we should change g_socket_service_add_listener() to
> g_socket_service_add_address(address, socket_type, protocol) and then
> add a helper function g_socket_service_add_inet_port() that does the
> ipv6/ipv4 ANY magic code.

I'm fine with that, but I think there is still cause for keeping the 
listener.


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




Thanks for the review.

Looking forward to your comments :)

Cheers