After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 728928 - Provide separate error code for "Connection reset by peer"
Provide separate error code for "Connection reset by peer"
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: network
2.38.x
Other Linux
: Normal enhancement
: ---
Assigned To: gtkdev
gtkdev
: 737705 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2014-04-25 06:31 UTC by Milan Crha
Modified: 2015-06-05 12:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gsocket: add G_IO_ERROR_CONNECTION_CLOSED (4.92 KB, patch)
2014-11-23 18:20 UTC, Dan Winship
committed Details | Review

Description Milan Crha 2014-04-25 06:31:15 UTC
All the internal processing hiding behind GInputStream is nice, but what I miss is better error checking. For example, a very specific error "Connection reset by peer" is returned as G_IO_ERROR_FAILED, which is... well... useless. It would be good to provide a "backend" specific error codes. For example, a GTcpConnection/GTlsConnection/GSocket/... should provide their own error domains and codes, thus if I receive G_IO_ERROR_GNUTLS, then I know the code is from the GnuTLS pool and I can distinguish between many fine-grained error codes.

As an initial step, it would be nice to at least provide G_IO_ERROR_CONNECTION_RESET_BY_PEER and/or
G_IO_ERROR_DISCONNECTED
, beside existing G_IO_ERROR_CONNECTION_REFUSED.
Comment 1 Dan Winship 2014-10-01 13:59:44 UTC
*** Bug 737705 has been marked as a duplicate of this bug. ***
Comment 2 Dan Winship 2014-10-01 14:09:29 UTC
I had started working on this a while back, but it turns out that this is a huge mess, portability-wise, so we need to be careful about what error codes we add, and what we promise about them.

In particular, whether you get EPIPE or ECONNRESET when writing to a closed socket appears to depend on both (a) what OS you're on, and (b) whether the remote end has acked all of the data you previously wrote.

Given that, it might make sense to have only a single error code covering both cases; for compatibility, this would have to be the existing G_IO_ERROR_BROKEN_PIPE, but we could add an alias for it as G_IO_ERROR_SOCKET_DISCONNECTED...

I've pushed an old branch I had lying around to wip/danw/socket-connected. IIRC, the test that it adds passes on Linux, but not on OS X. (That branch also changes the semantics of GSocket:connected in a way that I think we probably don't actually want, but I didn't bother to rebase out that commit.)
Comment 3 Dan Winship 2014-11-23 18:20:05 UTC
Created attachment 291320 [details] [review]
gsocket: add G_IO_ERROR_CONNECTION_CLOSED

Add G_IO_ERROR_CONNECTION_CLOSED as an alias for
G_IO_ERROR_BROKEN_PIPE, and also return it on ECONNRESET.

It doesn't really make sense to try to distinguish EPIPE and
ECONNRESET at the GLib level, since the exact choice of which error
gets returned in what conditions depends on the OS. Given that, we
ought to map the two errors to the same value, and since we're already
mapping EPIPE to G_IO_ERROR_BROKEN_PIPE, we need to map ECONNRESET to
that too. But the existing name doesn't really make sense for sockets,
so we add a new name.


==========


This is what I'm thinking about committing now. Thoughts?
Comment 4 Paolo Borelli 2014-11-23 21:18:40 UTC
Review of attachment 291320 [details] [review]:

It looks good to me though maybe it would be clearer to do the alias the other way round:

enum {
...
PIPE = CONNECTION_CLOSED /* for backward compatibility */
}


And if does not cause too much churn it would be good to use CONNECTION_CLOSED in all the internal uses of ERROR_PIPE (maybe in a follow up patch?)
Comment 5 Dan Winship 2014-11-29 19:37:13 UTC
> It looks good to me though maybe it would be clearer to do the alias
> the other way round:

I decided against this. They have to stay in the current order in the
docs, and it didn't seem right to have the declaration declare them in
the opposite order from the docs.

> And if does not cause too much churn it would be good to use
> CONNECTION_CLOSED in all the internal uses of ERROR_PIPE (maybe in a
> follow up patch?)

It turns out that there are no internal uses of BROKEN_PIPE (other
than the EPIPE -> G_IO_ERROR_BROKEN_PIPE mapping, which should stay as
BROKEN_PIPE), so no, it didn't cause too much churn. :)

Attachment 291320 [details] pushed as 967fedc - gsocket: add G_IO_ERROR_CONNECTION_CLOSED
Comment 6 Paolo Borelli 2014-11-29 20:51:38 UTC
should BROKEN_PIPE be marked as deprecated?
Comment 7 Dan Winship 2014-11-29 22:33:06 UTC
Well, if you're using GUnixOutputStream on a pipe, then BROKEN_PIPE really would be the right error code. Although I guess CONNECTION_CLOSED is a perfectly reasonable name for that condition too...

I have no strong opinion either way I guess.