Review of gnio, round 1

classic Classic list List threaded Threaded
37 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Review of gnio, round 1

Alexander Larsson
With gnome 2.26 out and the GResolver branch landed it is time to start
look at merging the gnio network code into gio. I'm posting this here,
plus CC:ing the involved people instead of on bugzilla in order to get
feedback from others who may be interested but unaware of this work.

This review is based on the gnio module in git.gnome.org, as of today,
which in turn is based on the current glib from master.

gnio has a two level design, the lowlevel GSocket is (in combination
with the already landed stuff) a lowlevel wrapper of the system socket
library, removing all system specific code etc from the higher levels.
Then there is a set of higher level utility classes. I'll start with a
detailed review of the GSocket code, because I think that part is
closest to landing. Then I'll give my thoughts on the current highlevel
stuff, which I think will need some restructuring. Maybe we should even
land the GSocket code before the highlevel code.

Some general comments on the code:

Almost no API docs, this is a blocker.

There is a lot of G_UNLIKELY() calls in places that really are not in
any way performance sensitive. I'm not sure I like this, since it makes
the code harder to read for very little gain. Like, it probably makes
sense for the inlined version of g_once_init_enter() to use G_LIKELY,
but I don't think it makes sense for e.g. error checking when creating a
socket, which is not gonna happen a lot.

GSocket.c:
==========

The error handling in GSocket is pretty bad. There is a "sticky" error
on it that you can read with a g_socket_has_error() call. This error is
set on construction errors, but also when i/o operations on the socket
fail. If this is set all operations just fail with this error. This is a
total fail, consider for instance a server accepting incomming
connections on a socket, if one accept call fails due to some transient
network problem then all further connections will be ignored.

This is clearly bogus, but I don't think we can totally remove it,
because construction errors can still happen, for instance you might
request an ipv6 socket but the OS doesn't support ipv6. So, I propose
that we turn the g_socket_has_error error into a pure construction
failed check. Also, we don't want to return that specific error from
other calls as the exact error may be confusing comming from some
method. Instead other calls should return FAILED with the string being
something like "socket creation failed due to: %s".

I don't really like the name g_socket_has_error() for checking for
construction failure though, as it seems a bit generic. Is there a
commonly used naming pattern for gobject construction failure checks?

Every time I've done socket code I've enabled SO_REUSEADDR, as without
this testing is a total pain in the ass. So, I think we should probably
turn this on by default. There is a very minor problem with it where a
old connection to the old closed socket can be confused with the new
socket, but this is extremely unlikely, see:
http://www.unixguide.net/network/socketfaq/4.5.shtml

However, it turns out that SO_REUSEADDR means something different on
windows. There if you enable this someone else can bind to the same
address even if your app is still actively bound to it. See:
http://cygwin.com/ml/cygwin/2004-10/msg00257.html
This is a much worse security issue, and I don't want to allow that by
default. So, the pragmatic approach is IMHO to enable it by default on
unix and disable on win32.

Win32 issues:
* g_socket_details_from_fd() is totally disabled for win32, which is not
right, but it needs some fixes for win32.
* Need to check errors from WSAStartup
* sendmsg is WSASendMsg on win32, needs special handling
* winsock doesn't set errno, need to call WSAGetLastError

"backlog" property needs getter and setter methods. Also, should
probably be called "listen_backlog" to describe it better.

We don't currently allow passing in the protocol to the socket. This
means we can't for instance use SCTP (the upcomming TCP replacement)
with GSocket. I think we should expose this via an optional char
*protocol argument that we look up via getprotobyname().

g_socket_finalize closes socket even if g_socket_close() is already
closed, need to add an is_closed variable. We should also check this in
all operations so that we don't accidentally call stuff on an fd that
may have been reused by now.

close needs to check for errors

g_socket_bind sinks the address, but socket addresses are not floating
anymore.

g_socket_send/recieve_message passes "flags" as is to the underlying
socket lib. This isn't exactly easy to use, but I can't really think of
a better approach. Any ideas?

g_socket_send_message should handle num_messages == -1 by looking for a
NULL termination in the array. (Similar to other glib apis)

set_blocking/set_reuse() etc functions don't have any return value atm.
Is that really right? What if they fail? Is it possible in practice for
them to fail for other than due to programmer errors?

There is some weird g_warnings about "ABI compatibility" that don't
really make sense to show the users.

There is IMHO some overuse of g_alloca. I mean, maybe its a useful
optimization in the case of a dynamic array, but there is some use of it
for constant length data instead of just using a local variable... Also,
even in the other cases there is a risk of using alloca where we don't
control the size as it can blow the stack.

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.

Having e.g. g_io_stream_get_output_stream() fallback on the property in
case no vfunc is specified in the interface is IMHO pretty weird. A much
more natural way would be to have the property implemented in the common
code, based on the get_output_stream vfunc. This would be more efficient
and need less code in inheriting classes. However, this is only possible
if we change GIOStream to a baseclass instead of an interface.

"if G_UNLIKELY (g_once_init_enter (&my_type))"
g_once_init_enter is already inline and has LIKELY annotations.

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.

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.

gnioerror.h:
------------

None of these are used anymore

GSocketInput/OutputStream:
--------------------------
Need to check is_cancelled before doing i/o

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


Highlevel API
=============

Then we come to the highlevel stuff. My basic problem here is that
exploding the socket types into different types for each of the
highlevel objects (TcpClient, UnxiClient, WhateverClient, etc) is just
wrong. First of all it hugely increases the apparent complexity of the
API creating lots of classes you have to use (adding casting etc) that
are more or less useless. Many are empty, and some just have a few
helper functions. And secondly, it doesn't really work like the lowlevel
abstraction, we just have a socket type, no unixsocket, tcpsocket,
sctpsocket, etc. So, why should we have that at the higher level.

I think we should just have:
GSocketConnection, GSocketService and possibly GSocketListener.

Lets go over each set of classes:

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.

GSocketConnection, GTcpConnection, GUnixConnection
==================================================

GTcpConnection is empty, so just remove. 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.

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.

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.

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

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.

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.


_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Behdad Esfahbod-3
On 04/27/2009 09:53 AM, Alexander Larsson wrote:
> With gnome 2.26 out and the GResolver branch landed it is time to start
> look at merging the gnio network code into gio. I'm posting this here,
> plus CC:ing the involved people instead of on bugzilla in order to get
> feedback from others who may be interested but unaware of this work.

Thanks Alex.


> There is a lot of G_UNLIKELY() calls in places that really are not in
> any way performance sensitive. I'm not sure I like this, since it makes
> the code harder to read for very little gain. Like, it probably makes
> sense for the inlined version of g_once_init_enter() to use G_LIKELY,
> but I don't think it makes sense for e.g. error checking when creating a
> socket, which is not gonna happen a lot.

I think I can answer this part since I use G_UNLIKELY extensively myself.  The
way I see it, it's not just a hint for the compiler, it's also an annotation
of the source code for the next person reading it, saying "this branch is
expected to happen in exceptional cases only, not the norm."  And I find it
extremely useful for that.


> Having e.g. g_io_stream_get_output_stream() fallback on the property in
> case no vfunc is specified in the interface is IMHO pretty weird. A much
> more natural way would be to have the property implemented in the common
> code, based on the get_output_stream vfunc. This would be more efficient
> and need less code in inheriting classes. However, this is only possible
> if we change GIOStream to a baseclass instead of an interface.

Not sure I'm following.  But wanted to point out making decisions based on
whether a vfunc is NULL is a very bad idea for bindings.

behdad
_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Ryan Lortie
In reply to this post by Alexander Larsson
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
_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Havoc Pennington-3
In reply to this post by Behdad Esfahbod-3
Hi,

On Mon, Apr 27, 2009 at 10:40 AM, Behdad Esfahbod <[hidden email]> wrote:

>> There is a lot of G_UNLIKELY() calls in places that really are not in
>> any way performance sensitive. I'm not sure I like this, since it makes
>> the code harder to read for very little gain. Like, it probably makes
>> sense for the inlined version of g_once_init_enter() to use G_LIKELY,
>> but I don't think it makes sense for e.g. error checking when creating a
>> socket, which is not gonna happen a lot.
>
> I think I can answer this part since I use G_UNLIKELY extensively myself.
>  The way I see it, it's not just a hint for the compiler, it's also an
> annotation of the source code for the next person reading it, saying "this
> branch is expected to happen in exceptional cases only, not the norm."  And
> I find it extremely useful for that.
>

I don't really know much about modern processors or compilation, but
my understanding is that the penalty for being wrong on G_UNLIKELY()
is quite high. And as we know from the old "premature optimization"
cliche it can be a bit hard to predict which codepaths get hit. For
example, I can certainly imagine uses of IO or socket APIs where IO
errors are hit reasonably often.

I thought G_UNLIKELY was really exclusively for cases where we "know"
the unlikely case will almost never be hit, like one-time
initialization, and that it was discouraged in cases where it's just
probabilistic, like "this is probably a bit less common"

But somebody should probably ask some gcc gurus

Havoc
_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Dan Winship-2
In reply to this post by Ryan Lortie
Ryan Lortie wrote:
> 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.

I can't think of any time I've used SHUT_RD or SHUT_WR rather than
SHUT_RDWR though...

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

Right, but it's disabled (at the kernel level) by default on most OSes
(everything but Linux?) because apparently the behavior is
underspecified. (Eg, see
http://www.potaroo.net/ietf/idref/draft-cmetz-v6ops-v4mapped-api-harmful/.)

So to avoid separate Linux-vs-everyone-else codepaths, it's probably
best to use IPV6_V6ONLY and manage v4 and v6 sockets separately everywhere.

-- Dan
_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Behdad Esfahbod-3
In reply to this post by Havoc Pennington-3
On 04/27/2009 11:40 AM, Havoc Pennington wrote:

> Hi,
>
> On Mon, Apr 27, 2009 at 10:40 AM, Behdad Esfahbod<[hidden email]>  wrote:
>>> There is a lot of G_UNLIKELY() calls in places that really are not in
>>> any way performance sensitive. I'm not sure I like this, since it makes
>>> the code harder to read for very little gain. Like, it probably makes
>>> sense for the inlined version of g_once_init_enter() to use G_LIKELY,
>>> but I don't think it makes sense for e.g. error checking when creating a
>>> socket, which is not gonna happen a lot.
>> I think I can answer this part since I use G_UNLIKELY extensively myself.
>>   The way I see it, it's not just a hint for the compiler, it's also an
>> annotation of the source code for the next person reading it, saying "this
>> branch is expected to happen in exceptional cases only, not the norm."  And
>> I find it extremely useful for that.
>>
>
> I don't really know much about modern processors or compilation, but
> my understanding is that the penalty for being wrong on G_UNLIKELY()
> is quite high. And as we know from the old "premature optimization"
> cliche it can be a bit hard to predict which codepaths get hit. For
> example, I can certainly imagine uses of IO or socket APIs where IO
> errors are hit reasonably often.
>
> I thought G_UNLIKELY was really exclusively for cases where we "know"
> the unlikely case will almost never be hit, like one-time
> initialization, and that it was discouraged in cases where it's just
> probabilistic, like "this is probably a bit less common"

My use case is mostly for 1) programmer errors, and 2) alloc failure errors.
Both of which are safe to assume they never happen.

behdad

> But somebody should probably ask some gcc gurus
>
> Havoc

_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

John McCutchan-3
In reply to this post by Havoc Pennington-3
On Mon, Apr 27, 2009 at 8:40 AM, Havoc Pennington
<[hidden email]> wrote:

> Hi,
>
> On Mon, Apr 27, 2009 at 10:40 AM, Behdad Esfahbod <[hidden email]> wrote:
>>> There is a lot of G_UNLIKELY() calls in places that really are not in
>>> any way performance sensitive. I'm not sure I like this, since it makes
>>> the code harder to read for very little gain. Like, it probably makes
>>> sense for the inlined version of g_once_init_enter() to use G_LIKELY,
>>> but I don't think it makes sense for e.g. error checking when creating a
>>> socket, which is not gonna happen a lot.
>>
>> I think I can answer this part since I use G_UNLIKELY extensively myself.
>>  The way I see it, it's not just a hint for the compiler, it's also an
>> annotation of the source code for the next person reading it, saying "this
>> branch is expected to happen in exceptional cases only, not the norm."  And
>> I find it extremely useful for that.
>>
>
> I don't really know much about modern processors or compilation, but
> my understanding is that the penalty for being wrong on G_UNLIKELY()
> is quite high. And as we know from the old "premature optimization"
> cliche it can be a bit hard to predict which codepaths get hit. For
> example, I can certainly imagine uses of IO or socket APIs where IO
> errors are hit reasonably often.
>

The penalty for being wrong is the same as when the CPU makes a bad
branch prediction.

You are providing a hint though and if that hint is bad, you are going
to cause the CPU to initially take the wrong branch, which can be
expensive.

It's good practice to avoid using hints unless you have a firm grasp
on the odds of the branch being taken.

--
John McCutchan <[hidden email]>
_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Dan Winship-2
In reply to this post by Alexander Larsson
(I still haven't read all of gnio, so I'm mostly only commenting on
Alex's comments, plus GSocket, since I have read that.)

Alexander Larsson wrote:
> Every time I've done socket code I've enabled SO_REUSEADDR

Yeah, Ryan and I had this exact conversation (including the same link)
on IRC yesterday.

There are also other flags we might want to think about. In libsoup, all
sockets get marked FD_CLOEXEC because of a long-ago bug. (The Red Carpet
daemon used librpm to do stuff, and librpm would spawn postinstall
scripts without doing the necessary cleanup like closing fds, and so
sometimes you'd try to restart rcd, and it would fail to start up again,
because some process like cupsd which had previously been restarted by
rcd now was holding a leaked copy of the old rcd process's listening
socket, and so the new rcd process couldn't re-bind that address.)
Anyway, ideally people would use spawning APIs that cleaned up fds
properly, but the fact is, unless you're writing inetd, you generally
don't intend to hand off your sockets to child processes anyway, so
setting FD_CLOEXEC unconditionally (or using SOCK_CLOEXEC where
available) generally makes sense.

> Win32 issues:

I'd filed a bug about some other win32 issues over the weekend.
(http://bugzilla.gnome.org/show_bug.cgi?id=580236)

> * Need to check errors from WSAStartup

Would it be better to have only one place in gio that calls WSAStartup?
(Currently that's g_inet_address_get_type().)

> We don't currently allow passing in the protocol to the socket. This
> means we can't for instance use SCTP (the upcomming TCP replacement)
> with GSocket.

I imagine the theory is that G_SOCKET_TYPE_STREAM,
G_SOCKET_TYPE_DATAGRAM, and G_SOCKET_TYPE_SEQPACKET invariably also
imply tcp, udp, and sctp, respectively.

> g_socket_finalize closes socket even if g_socket_close() is already
> closed, need to add an is_closed variable. We should also check this in
> all operations so that we don't accidentally call stuff on an fd that
> may have been reused by now.

libsoup's solution here is to implement "close" as
shutdown(fd,SHUT_RDWR), and only actually call close(fd) at finalization
time.

> There is some weird g_warnings about "ABI compatibility" that don't
> really make sense to show the users.

Yeah, we should ensure that at compile time and remove the checks.

> GSocketConnection *g_socket_connection_connect_to_host (const gchar
> *host,
>                                                         const gchar
> *default_port,
> GCancellable        *cancellable,
>   GError             **error);

IMHO, ports should always be numbers, not strings. getservbyname() is
useless, because you want your app to work on last year's distro that
doesn't know about this year's protocols. So apps should just provide a
#define for their port number and use that, rather than hoping that the
service's name will be in /etc/services.

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

My vision of how this works is something like:

    - GUnixSocket{Connection,Service} - does unix sockets. Has APIs for
      using abstract names, but they return G_IO_ERROR_NOT_SUPPORTED on
      non-Linux.

    - GWin32NamedPipe{Connection,Service} - a Windows named pipe.
      Implements GIOStream, but is not related to any of the socket
      types.

    - GNamedBidirectionalIPC{Connection,Service} - Takes an
      application name and a channel name, and a flag for "session" or
      "system" level, and then "it does something", such that if one
      app makes the server and another app makes a client with the
      same parameters, then they can talk to each other. On Windows,
      this would use a named pipe, on Linux it would use abstract
      sockets, on other unixes, it would use a unix socket in /tmp.
      Or maybe it passes fds over D-Bus. And maybe on OS X it uses
      Mach ports.

So then, if you want to talk to an app that uses unix sockets, you
create a GUnixSocketConnection (and you don't need to worry about
fallback, because obviously you're not going to be trying to connect to
a service running on an abstract socket on a platform that doesn't
support them). And likewise if you want to connect to a windows named
pipe, you create a GWin32NamedPipeConnection. But if your goal is just
"make it possible for other apps to communicate with my app efficiently
in a standardized way", then you use the abstract service class, and it
does the right thing for your platform, and the client apps use the
abstract connection class, and it knows how to connect.

-- Dan
_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Mark Mielke
In reply to this post by John McCutchan-3
John McCutchan wrote:
On Mon, Apr 27, 2009 at 8:40 AM, Havoc Pennington
[hidden email] wrote:
  
I don't really know much about modern processors or compilation, but
my understanding is that the penalty for being wrong on G_UNLIKELY()
is quite high. And as we know from the old "premature optimization"
cliche it can be a bit hard to predict which codepaths get hit. For
example, I can certainly imagine uses of IO or socket APIs where IO
errors are hit reasonably often.
    

The penalty for being wrong is the same as when the CPU makes a bad
branch prediction.
  

Hi John:

On the penalty being the same - are you assuming or have you looked at the generated assembly?

The last time I tried this out and analyzed gcc -S output, the case without using the "likely" hint generated code that still used a branch, but was mostly inline in both cases. When I switched to using a "likely" hint, it moved the entire "unlikely" block down to the bottom of the function. Yes, there is a bad branch prediction cost in both cases, but I think there is *also* a cache miss possibility in here. That is, if the case is truly unlikely - it's been moved out to the bottom into a different cache line, leaving more room in the calling cache line for the success case.

Meaning - you really do want the "unlikely" case to be "unlikely" before you tell the compiler that it is "unlikely". The compiler will try to improve the speed of the expected case at the expense of the not expected case.

It's good practice to avoid using hints unless you have a firm grasp
on the odds of the branch being taken.
  

Definitely. For this thread in specific - what might be unexpected today might become expected tomorrow. Using it only in specific cases where you know for certain the compiler has a chance of generating bad code (hopefully from analysis of the generated code or analysis of run time traces) is the best model. Let the compiler do its job for the rest. It can probably out-think you, and defining a block as unlikely might limit it's ability to reorder your code to best advantage.

Cheers,
mark

-- 
Mark Mielke [hidden email]

_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

John McCutchan-3
On Mon, Apr 27, 2009 at 10:30 AM, Mark Mielke <[hidden email]> wrote:

> John McCutchan wrote:
>
> On Mon, Apr 27, 2009 at 8:40 AM, Havoc Pennington
> <[hidden email]> wrote:
>
>
> I don't really know much about modern processors or compilation, but
> my understanding is that the penalty for being wrong on G_UNLIKELY()
> is quite high. And as we know from the old "premature optimization"
> cliche it can be a bit hard to predict which codepaths get hit. For
> example, I can certainly imagine uses of IO or socket APIs where IO
> errors are hit reasonably often.
>
>
> The penalty for being wrong is the same as when the CPU makes a bad
> branch prediction.
>
>
> Hi John:
>
> On the penalty being the same - are you assuming or have you looked at the
> generated assembly?
>
> The last time I tried this out and analyzed gcc -S output, the case without
> using the "likely" hint generated code that still used a branch, but was
> mostly inline in both cases. When I switched to using a "likely" hint, it
> moved the entire "unlikely" block down to the bottom of the function. Yes,
> there is a bad branch prediction cost in both cases, but I think there is
> *also* a cache miss possibility in here. That is, if the case is truly
> unlikely - it's been moved out to the bottom into a different cache line,
> leaving more room in the calling cache line for the success case.
>

My commentary was based on looking at the assembly code and gcc didn't
move functions around with my tests. Your point is an excellent one,
incorrect use of "likely" and "unlikely" could easily lead to an
i-cache miss.

--
John McCutchan <[hidden email]>
_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Havoc Pennington-2
In reply to this post by John McCutchan-3
Hi,

On Mon, Apr 27, 2009 at 1:20 PM, John McCutchan <[hidden email]> wrote:
> It's good practice to avoid using hints unless you have a firm grasp
> on the odds of the branch being taken.
>

The thing I find kind of intractable, is that you have to not only
know the odds, but think your grasp is better than what the compiler
and CPU are going to come up with by default.
Personally, I have no real way to evaluate that, except in cases where
I know one branch is basically a "run once" or otherwise pretty much
never happens.

I mean, say you think a branch happens 10% of the time. Is it a net
win to mark that UNLIKELY or will the 10% of the time be so much worse
that it outweighs the 90% of the time being faster? How about 5% of
the time? 30% of the time? What does "unlikely" mean?

And what heuristics do the compiler and cpu already use?

It's just not at all clear. I've read some people's code where it
seems any less-than-50%-likely case gets marked G_UNLIKELY. And GLib
itself I think only marks the "one-time or should-never-happen" cases,
or that's my impression. And then some people seem to be marking "10%
of the time or so" as unlikely.

Limiting it to one-time-or-should-never-happen, or inner loops that
have in fact been profiled, would seem to follow the "no premature
optimization" rule.

Havoc
_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Alexander Larsson
In reply to this post by Ryan Lortie
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.


_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Alexander Larsson
In reply to this post by Dan Winship-2
On Mon, 2009-04-27 at 11:55 -0400, Dan Winship wrote:
> Ryan Lortie wrote:
> > 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.
>
> I can't think of any time I've used SHUT_RD or SHUT_WR rather than
> SHUT_RDWR though...

You can use SHUT_WR to get reliable socket shutdown, making sure the
remote side got all the data (without using SO_LINGER). See e.g.:

http://blog.netherlabs.nl/articles/2009/01/18/the-ultimate-so_linger-page-or-why-is-my-tcp-not-reliable

Although this kind of thing looks like something the highlevel code
should support not by exposing shutdown, but rather by having a function
that does all of this.

> >> 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.
>
> Right, but it's disabled (at the kernel level) by default on most OSes
> (everything but Linux?) because apparently the behavior is
> underspecified. (Eg, see
> http://www.potaroo.net/ietf/idref/draft-cmetz-v6ops-v4mapped-api-harmful/.)
>
> So to avoid separate Linux-vs-everyone-else codepaths, it's probably
> best to use IPV6_V6ONLY and manage v4 and v6 sockets separately everywhere.

As per http://people.redhat.com/drepper/userapi-ipv6.html this doesn't
work for linux, as it only allows one socket per port. Isn't portable
programming fun!

_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Alexander Larsson
In reply to this post by Dan Winship-2
On Mon, 2009-04-27 at 13:22 -0400, Dan Winship wrote:

> There are also other flags we might want to think about. In libsoup, all
> sockets get marked FD_CLOEXEC because of a long-ago bug. (The Red Carpet
> daemon used librpm to do stuff, and librpm would spawn postinstall
> scripts without doing the necessary cleanup like closing fds, and so
> sometimes you'd try to restart rcd, and it would fail to start up again,
> because some process like cupsd which had previously been restarted by
> rcd now was holding a leaked copy of the old rcd process's listening
> socket, and so the new rcd process couldn't re-bind that address.)
> Anyway, ideally people would use spawning APIs that cleaned up fds
> properly, but the fact is, unless you're writing inetd, you generally
> don't intend to hand off your sockets to child processes anyway, so
> setting FD_CLOEXEC unconditionally (or using SOCK_CLOEXEC where
> available) generally makes sense.

There is also SO_KEEPALIVE which seems to see some use. And possibly
SO_LINGER, although that seems fraught with problems.

> > Win32 issues:
>
> I'd filed a bug about some other win32 issues over the weekend.
> (http://bugzilla.gnome.org/show_bug.cgi?id=580236)

More todo... Need to get some win32 people involved to test this stuff.

> > * Need to check errors from WSAStartup
>
> Would it be better to have only one place in gio that calls WSAStartup?
> (Currently that's g_inet_address_get_type().)

Yes, i forgot that. I think thats a good place. I'll fix that.

> > We don't currently allow passing in the protocol to the socket. This
> > means we can't for instance use SCTP (the upcomming TCP replacement)
> > with GSocket.
>
> I imagine the theory is that G_SOCKET_TYPE_STREAM,
> G_SOCKET_TYPE_DATAGRAM, and G_SOCKET_TYPE_SEQPACKET invariably also
> imply tcp, udp, and sctp, respectively.

Well, you can use sctp in the one-to-one mode with SOCK_STREAM too. I'll
add the protocol argument so the user can explicitly pick one.

> > g_socket_finalize closes socket even if g_socket_close() is already
> > closed, need to add an is_closed variable. We should also check this in
> > all operations so that we don't accidentally call stuff on an fd that
> > may have been reused by now.
>
> libsoup's solution here is to implement "close" as
> shutdown(fd,SHUT_RDWR), and only actually call close(fd) at finalization
> time.

Whats the advantage here over close() and then setting some is_closed
bit?

> > GSocketConnection *g_socket_connection_connect_to_host (const gchar
> > *host,
> >                                                         const gchar
> > *default_port,
> > GCancellable        *cancellable,
> >   GError             **error);
>
> IMHO, ports should always be numbers, not strings. getservbyname() is
> useless, because you want your app to work on last year's distro that
> doesn't know about this year's protocols. So apps should just provide a
> #define for their port number and use that, rather than hoping that the
> service's name will be in /etc/services.

I agree.

> > 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.
>
> My vision of how this works is something like:
>
>     - GUnixSocket{Connection,Service} - does unix sockets. Has APIs for
>       using abstract names, but they return G_IO_ERROR_NOT_SUPPORTED on
>       non-Linux.
>
>     - GWin32NamedPipe{Connection,Service} - a Windows named pipe.
>       Implements GIOStream, but is not related to any of the socket
>       types.
>
>     - GNamedBidirectionalIPC{Connection,Service} - Takes an
>       application name and a channel name, and a flag for "session" or
>       "system" level, and then "it does something", such that if one
>       app makes the server and another app makes a client with the
>       same parameters, then they can talk to each other. On Windows,
>       this would use a named pipe, on Linux it would use abstract
>       sockets, on other unixes, it would use a unix socket in /tmp.
>       Or maybe it passes fds over D-Bus. And maybe on OS X it uses
>       Mach ports.

I like the basic idea, but.... soo ... many ... classes... :)



_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Tor Lillqvist
In reply to this post by Alexander Larsson
> 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.

It was on the dbus list. Quoting myself and Thiago Macieira:

> Em Terça-feira 21 Abril 2009, às 16:32:15, Tor Lillqvist escreveu:
>> My guess is that a Windows equivalent to Unix domain sockets would be
>> named pipes? (Should not be confused with Unix named pipes; Only the
>> name is the same.)

2009/4/21 Thiago Macieira <[hidden email]>:

> That's what we used for the Windows version of QLocalSocket
> (http://doc.trolltech.com/4.5/qlocalsocket.html), but now that we have used
> it, I wouldn't recommend it for D-Bus.
>
> First of all, they're a bit slow. We have an open suggestion in our task
> tracker system to replace the backend with completion ports. I have not yet
> investigated what those things are, so I can't comment on them, except that
> the person who made the suggestion thinks they're faster.
>
> Second, named pipes on Windows are very hard to use. Named pipes use a
> separate namespace on the filesystem hierarchy than files. And unlike a TCP
> server socket or Unix server socket, one file descriptor cannot accept multiple
> connections. Instead, the daemon must create a series of backup pipes with the
> same name. That's not such a big problem, if you consider the "backlog"
> argument to listen(2).
>
> But named pipes only get worse: Windows doesn't have the concept of non-
> blocking calls like Unix does (except for sockets, which are technically
> imported from BSD). The "overlapped" concept is similar, but not exactly. And
> to make matters worse, you can't get a notification that the HANDLE is ready to
> write: so you need to keep a thread running that constantly tries to write.
>
> And if you're not satisfied, then here's one more: named pipes are not
> available on Windows CE. So the Windows CE version of QLocalSocket uses...
> TCP/IP :-)
> [So I would actually go as far as implement the nonce+TCP/IP suggestion as a
> fallback for QLocalSocket itself, to match what D-Bus and libassuan do.

--tml
_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Alexander Larsson
In reply to this post by Havoc Pennington-2
On Mon, 2009-04-27 at 15:24 -0400, Havoc Pennington wrote:
> Limiting it to one-time-or-should-never-happen, or inner loops that
> have in fact been profiled, would seem to follow the "no premature
> optimization" rule.

This seems like a good rule of thumb to me.

_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Dan Winship-2
In reply to this post by Alexander Larsson
Alexander Larsson wrote:

> On Mon, 2009-04-27 at 13:22 -0400, Dan Winship wrote:
>>> g_socket_finalize closes socket even if g_socket_close() is already
>>> closed, need to add an is_closed variable. We should also check this in
>>> all operations so that we don't accidentally call stuff on an fd that
>>> may have been reused by now.
>> libsoup's solution here is to implement "close" as
>> shutdown(fd,SHUT_RDWR), and only actually call close(fd) at finalization
>> time.
>
> Whats the advantage here over close() and then setting some is_closed
> bit?

Well, you don't need an is_closed bit, and you don't need to add a
special check to every op. You just try to recv() or send() or whatever,
and get back ENOTCONN (or whatever it is), and handle that normally just
like if it had been the remote end that closed the connection.

In libsoup it's also important because it's thread-safe/non-racy. That
may not be a relevant criterion for GSocket, although the source
returned by g_socket_create_source() could create similar problems; you
need to be certain that any such sources are destroyed before you call
g_socket_close(), or else they could trigger falsely if the fd gets
reused. Delaying close() until finalization makes that easy because then
you can just have the source hold a ref on the socket to ensure that the
fd doesn't get closed.

-- Dan
_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Mark Mielke
Dan Winship wrote:
Alexander Larsson wrote:
  
On Mon, 2009-04-27 at 13:22 -0400, Dan Winship wrote:
    
g_socket_finalize closes socket even if g_socket_close() is already
closed, need to add an is_closed variable. We should also check this in
all operations so that we don't accidentally call stuff on an fd that
may have been reused by now.
        
libsoup's solution here is to implement "close" as
shutdown(fd,SHUT_RDWR), and only actually call close(fd) at finalization
time.
      
Whats the advantage here over close() and then setting some is_closed
bit?
    

Well, you don't need an is_closed bit, and you don't need to add a
special check to every op. You just try to recv() or send() or whatever,
and get back ENOTCONN (or whatever it is), and handle that normally just
like if it had been the remote end that closed the connection.
  

Leaving a file descriptor open has a cost. Calling I/O operations on a closed handle should give an E_PROGRAMMER_ERROR exception - not a system call errno. :-)

In libsoup it's also important because it's thread-safe/non-racy. That
may not be a relevant criterion for GSocket, although the source
returned by g_socket_create_source() could create similar problems; you
need to be certain that any such sources are destroyed before you call
g_socket_close(), or else they could trigger falsely if the fd gets
reused. Delaying close() until finalization makes that easy because then
you can just have the source hold a ref on the socket to ensure that the
fd doesn't get closed.
  

It's a clever solution to the thread-safe issue - but it doesn't seem like a great solution. Leaving file descriptors open "in case" they happen to still have references floating around in memory that might be used, and waiting until the object is removed or program termination before closing the file descriptor is a bit hacky / non-scaleable. I'm sure it works great if the program only uses one or two file descriptors.

Still, I can't think of a better solution for dealing with inherently buggy code that might accidentally use a file descriptor from another thread after it has been closed but before is_closed is set without locking the system down with mutexes which would become painful.

So, it's either use up file descriptors in case of a broken caller, or don't use up file descriptors, but require the caller to be written sensibly.

Cheers,
mark

-- 
Mark Mielke [hidden email]

_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Dave Benson
> >In libsoup it's also important because it's thread-safe/non-racy. That
> >may not be a relevant criterion for GSocket, although the source
> >returned by g_socket_create_source() could create similar problems; you
> >need to be certain that any such sources are destroyed before you call
> >g_socket_close(), or else they could trigger falsely if the fd gets
> >reused. Delaying close() until finalization makes that easy because then
> >you can just have the source hold a ref on the socket to ensure that the
> >fd doesn't get closed.

Why not just set the FD to -1?
It'll give bad a EBADF instead of ENOTCONNECTED (or whatever),
but it seems better...  no flags, i guess it's "racy" but that's
intrinsic to the circumstance of having one thread writing and another closing.
(Really, the caller is going to have to have a lock if they want
multiple threads to write to the same fd)


- dave
_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of gnio, round 1

Alexander Larsson
On Mon, 2009-04-27 at 17:28 -0700, Dave Benson wrote:

> > >In libsoup it's also important because it's thread-safe/non-racy. That
> > >may not be a relevant criterion for GSocket, although the source
> > >returned by g_socket_create_source() could create similar problems; you
> > >need to be certain that any such sources are destroyed before you call
> > >g_socket_close(), or else they could trigger falsely if the fd gets
> > >reused. Delaying close() until finalization makes that easy because then
> > >you can just have the source hold a ref on the socket to ensure that the
> > >fd doesn't get closed.
>
> Why not just set the FD to -1?
> It'll give bad a EBADF instead of ENOTCONNECTED (or whatever),
> but it seems better...  no flags, i guess it's "racy" but that's
> intrinsic to the circumstance of having one thread writing and another closing.
> (Really, the caller is going to have to have a lock if they want
> multiple threads to write to the same fd)

The race isn't about another thread writing to the socket, but rather a
mainloop source created from the socket, and that puts the fd in the
mainloop, so its not that trivial to "set it to -1".

_______________________________________________
gtk-devel-list mailing list
[hidden email]
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
12