Re: Review of gnio, round 2

[Alexander Larsson <alexl redhat com>]

On Mon, 2009-05-18 at 14:36 -0400, Dan Winship wrote:
> more comments, now that it's all committed... :-}

I'll fix up the stuff, but here are some comments:

> g_socket_new()'s documentation and implementation don't match wrt
> "protocol". (In particular, the docs refer to the "protocol_id" argument
> as "protocol", and talk about %NULL instead of 0.) Also, is there some
> reason this is an int rather than another GLIB_SYSDEF_*'ed enum? Also,
> it should be consistent throughout whether sockets have a "protocol" or
> a "protocol_id". (The property is currently "protocol", but everything
> else is "protocol_id".)

I'll fix up the inconsistencies.

There is no GLIB_SYSDEF enum thing because there are no such thing in
the sockets API. You use getprotobyname, which is wrapped as
g_socket_protocol_id_lookup_by_name().

> "gchar buffer[256]" as a generic sockaddr is wrong and you should feel
> bad for hardcoding a magic number. Use "struct sockaddr_storage", which
> is defined to be "big enough".

I didn't write that part, but I didn't bother to fix it (struct
sockaddr_storage is 128 bytes after all), but I guess we should clean it
up.

> I dislike g_socket_check_pending_error() as a name. Is it ever used for
> anything except the connect() result? (Google seems to suggest, no, not
> really.) Maybe g_socket_connect_finish()?

I don't know if its used for anything else. I don't like _finish since
it matches the _async/_finish scheme but works differently. Maybe
g_socket_check_connect_result() ?

> g_socket_close() may need to be revisited. In particular, if you close()
> a socket when there is unread data on it, it may cause the other end of
> the connection to receive an error and drop the connection before it
> finishes reading all of the data which you sent it. (This has not been a
> problem for libsoup because in HTTP it's always clear whose turn it is
> to talk, and so you won't generally end up closing a socket with unread
> data on it.) I am still figuring out the details here. qv
> http://74.125.45.132/search?q=cache:7iHzJ3hxy80J:cs.baylor.edu/~donahoo/practical/CSockets/TCPRST.pdf
> http://blog.netherlabs.nl/articles/2009/01/18/the-ultimate-so_linger-page-or-why-is-my-tcp-not-reliable

The way I see this is that GSocket is a direct mapping of the lowest
common denominator BSD socket API and how it behaves. As such, it maps
the weirdness of sockets directly. This particular issue is specific  to
TCP connections for a subset of applications (most protocols don't close
without an ack or pass explicit sizes so this is no problem), so I don't
think we should specifically tweak the GSocket API to handle it.

However, in order to allow a method in TcpConnection like
g_tcp_connection_set_graceful_close() we should probably expose the
shutdown call in GSocket.

Also, is the shutdown write, read waiting for 0, then close really
always safe. What if the other side doesn't close the socket upon
recieving the partial close? Then we can wait forever. Maybe this needs
to be part of the higher level protocol handling? At least it should be
optional.

> g_socket_create_source() says "cancellable if not NULL can be used to
> cancel the source, which will cause the source to trigger, reporting the
> current condition." First, this does not appear to be true; it looks
> like it will cause the source to trigger with condition==0. Second,
> shouldn't it trigger with G_IO_ERR anyway for consistency with other
> GCancellable usage? Maybe not...

Hmm, there are two implementations of this the win32 one in gsocket.c
and the gasynchelper.c one.

winsock_dispatch() passes:
winsock_source->result_condition & winsock_source->condition
where result_condition is set to the current value in winsock_check. 

fd_source_dispatch passes fd_source->pollfd.revents which is basically 
the result from the poll from the time of the poll exit.

Both of these seem to me to be more or less the "current" condition,
although at the time of the exit from the mainloop poll. However, in
very many cases (unless we're both cancelled and some condition was
fulfilled) the current value is in fact 0. 

So, i'm not sure how to best describe this.

> g_socket_get/set_keepalive() should probably be removed. SO_KEEPALIVE is
> apparently not all that useful. It only sends the probes every 2 hours
> (by default, controllable by sysctl), and these days, it's pretty
> uncommon to have a server that will keep a completely idle connection
> open for even that long. And people can use setsockopt() if they really
> need it...

I don't think its not useful. In server implementations for protocols
where the connection is idle unless the client sends something this is
quite important. The canonical example is a shell server, but the same
goes for e.g. a MUD or an echo service. 

Consider a client that connects to these services, sends some requests
and gets some responses, then goes idle and after a while falls of the
net (say the machine crashed). The server has no way to know that the 
client will be idle forever, so the connection will stay around forever
in the server, basically causing an undetectable resource leak. Turning
on keepalive will guarantee that the connection is eventually
terminated, even though it might not  happen immediately.

> (OTOH, TCP_NODELAY, SO_RCVTIMEO, and SO_SNDTIMEO may be more worthwhile,
> though TCP_NODELAY might want to be merged into a single API with
> TCP_CORK/MSG_MORE. More to think about...)

Yeah, but additions are easy to do later.