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 561547 - soup_server_new() might return NULL
soup_server_new() might return NULL
Status: RESOLVED FIXED
Product: libsoup
Classification: Core
Component: API
unspecified
Other Linux
: Normal normal
: ---
Assigned To: libsoup-maint@gnome.bugs
libsoup-maint@gnome.bugs
: 654449 702911 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2008-11-19 16:49 UTC by Andreas Rottmann
Modified: 2014-05-02 13:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
start SoupServer rewrite (17.70 KB, patch)
2009-06-08 13:41 UTC, Dan Winship
none Details | Review

Description Andreas Rottmann 2008-11-19 16:49:19 UTC
When (for example) libsoup couldn't grab the port specified, soup_server_new() returns NULL.

This is a problem because there's no error indication, not even on stdout. There should actually be a GError result in this case. 

I'm not sure it makes sense to do the whole setup in the GObject constructor, perhaps it would be better to have a separate method, e.g.:

   soup_server_listen(SoupServer *server, GError **);

This way, the reason for the error could be communicated to the caller, who could (for instance) reconfigure port settings or certificate file paths, and retry.
Comment 1 Dan Winship 2008-11-19 17:53:46 UTC
requiring a soup_server_listen() call would be an API break. In particular, note the subclass of SoupServer in bug 491639, which I believe is currently being used in an application elsewhere, so we do need to be careful to not break any current API contracts.

One possibility would be to add a construct-only write-only "construct-error" property, that takes a GError **, so you could do:

    server = soup_server_new (SOUP_SERVER_PORT, my_port,
                              SOUP_SERVER_SERVER_HEADER, "blah/1.0 ",
                              SOUP_SERVER_CONSTRUCT_ERROR, &error,
                              NULL);

("write-only" meaning you're "setting" it to the value &error, but really it doesn't cause any value to be set, it just gets used from within the constructor() vmethod.)

Another possibility would be to deprecate soup_server_new() in favor of a new constructor with different args, either explicitly including a GError, or else requiring a separate call to soup_server_listen().

The use of g_object_new()-like constructors in libsoup is turning out to be a problem with language bindings anyway. gobject-introspection doesn't really have any way to deal with them. So we need new/different constructors anyway...
Comment 2 Dan Winship 2008-11-19 17:56:41 UTC
the fix may also tie into bug 522519; we want to tweak SoupServer's port/interface properties anyway to allow a single server to listen on multiple interfaces (IPv4 and IPv6), do both http and https, etc.
Comment 3 Andreas Rottmann 2008-11-20 13:45:51 UTC
> The use of g_object_new()-like constructors in libsoup is turning out to be a
> problem with language bindings anyway. gobject-introspection doesn't really
> have any way to deal with them. So we need new/different constructors anyway...

Well, in sbank (Scheme gobject-introspection binding), I use the generic g_object_newv() under the hood for constructing a SoupServer (as soup_server_new() is just a wrapper around that anyway). But indeed, there is no way to bind vararg functions with g-i.

> Another possibility would be to deprecate soup_server_new() in favor of a new
> constructor with different args, either explicitly including a GError, or else
> requiring a separate call to soup_server_listen().

I would prefer the second solution here; changing the code like this:

* Add a new function soup_server_new_no_listen(),
  which just invokes g_object_new().

* Remove the GObject constructor from SoupServer, so no listening is performed 
  when g_object_newv() or soup_server_new_no_listen() is called.

* Add a new function soup_server_listen(SoupServer *server, GError **) that
  sets up the socket.

* Within soup_server_new(), call soup_server_new_no_listen(), followed by
  soup_server_listen(). If that fails, deallocate the instance and return NULL.

Comment 4 Andreas Rottmann 2008-12-07 19:18:44 UTC
Looking at the code, I found that the error information (from errno after bind()) is "swallowed" by the socket abstraction. I think such information should be always be made available to the user of a library. If I get a "go" from a libsoup dev, I'd like to work a bit on this topic; actually I have some code in the stalled GOIO project (http://home.gna.org/goio/), also LGPL-licensed, that deals properly with this stuff, I could import that into libsoup...
Comment 5 Dan Winship 2008-12-07 19:28:17 UTC
Well... there aren't all that many ways it can fail.

What sort of changes are you thinking of making? Again, the existing API can't be changed at this point.

(Also, not sure if you're aware of it, but there's a bug for adding socket methods to gio, bug 515973.)
Comment 6 Dan Winship 2009-04-23 17:44:02 UTC
vague outline of how this should work:

We want to solve this and bug 522519 (listening on multiple interfaces, listening to both SSL and non-SSL at the same time, etc) together. This will result in deprecating quite a lot of the SoupServer API. (Anything that assumes there's only one interface, one listening socket, one SSL status, etc.)

The code in SoupServer:constructor() should be moved to the "constructed" method rather than "constructor", since returning NULL from there is illegal according to GObject's rules anyway. It should NOT call soup_socket_listen(), and it should not set priv->interface if it wasn't already set.

set_property should set some flag if SOUP_SERVER_INTERFACE or SOUP_SERVER_PORT is set.

Add a new method or methods, soup_server_listen() or something, to start listening, and have proper error reporting. You should be able to call this multiple times, to listen on multiple sockets (eg, IPv4 and IPv6, multiple interfaces, SSL and non-SSL, etc). Do we need the ability to have multiple SSL certs? It would be a lot easier if we said no...

We'll also need some corresponding get/list methods...

get_property, soup_server_get_listener, soup_server_run_async, and soup_server_run need to be modified such that if the caller used SOUP_SERVER_INTERFACE/SOUP_SERVER_PORT, or if they didn't use either of those, but didn't call soup_server_listen() either, then it should do the proper setup to ensure backward-compatible behavior.
Comment 7 Andreas Rottmann 2009-06-06 23:12:54 UTC
An additional binding-related thought: In bindings for languages with garbage collection, you don't have explicit control over reference counts, so you cannot predict when the created SoupServer will be destroyed and free its resources (primarily the listening socket). 

Hence I think it would make sense to add a method like `soup_server_shutdown', which would be the counterpart to `soup_server_listen': it'd call `soup_server_quit', then give up the socket. After invoking shutdown, you could call `soup_server_listen' on the same server again (maybe also useful for C), or create a brand-new server on the same port, while waiting for the GC to kick in and free the memory of the old server (for the language binding usecase).
Comment 8 Andreas Rottmann 2009-06-07 20:13:07 UTC
(In reply to comment #6)
> vague outline of how this should work:
> 
> [...]
>
To sum it up: you want to delay the setup (socket listen, etc.) to the
point when actually required. I think there are still issues regarding
backwards compatibility with that approach, one major, and one minor:

1) As used in the test suite, where SoupServer is run in its own
   thread, we now get a race condition:
   
   The main thread calls `soup_test_server_new (TRUE)', which first
   constructs a SoupServer; with the above approach, that will not
   cause the socket to be bound. Now the server thread is spawned, and
   the SoupServer object returned to the main thread. The two threads
   are now racing for initialization of the socket, as the main thread
   calls `soup_server_get_port()' on the returned server object.

2) In a single threaded-case, the above is naturally not an issue, but
   there still might be code out there which expects any failure to
   happen on SoupServer construction. With the outlined approach, the
   failure will happen later, and there is no way to communicate the
   failure to the user with the current API, as now any of
   `soup_server_get_{interface,port,run,run_async}()', plus
   `get_property()' may fail. There is no way to handle it, but to do
   a `g_assert()' or something alike. I think this wouldn't be a big
   issue, as it would only affect code if something goes wrong in the
   first place.

I see no way around that without breaking backwards compatibility, but
perhaps you have an idea. 

With the approach I proposed (comment #3), you'd clearly break
backwards compatibility for subclasses and people who call
g_object_new(SOUP_TYPE_SERVER, ...), but at least you don't introduce
a race condition in multi-threaded code, and the semantics when the
socket initialization happens (and thus things might fail), are
clearer, and the resulting code would have less fuss, at least IMHO.

If we can't find a solution that does both (backwards compatibility
and allowing reasonable error handling for new code), this bug should
perhaps be targeted to be resolved as part of the GNOME 3 API break.
Comment 9 Dan Winship 2009-06-08 03:14:48 UTC
(In reply to comment #8)
> 1) As used in the test suite, where SoupServer is run in its own
>    thread, we now get a race condition:

Sigh. Of course, technically, that code is wrong, since SoupServer is not supposed to be thread-safe, but yeah. (We could work around this by adding mutexes to be used in the backward-compatible-API case...)

> 2) In a single threaded-case, the above is naturally not an issue, but
>    there still might be code out there which expects any failure to
>    happen on SoupServer construction.

Hm... I was thinking such code was mistaken because GObject would just crash if SoupServer's constructor() returned NULL. But yeah, it seems to work... (I think there are *some* cases where returning NULL from constructor() causes GObject to crash, but I guess we don't hit them.)

> If we can't find a solution that does both (backwards compatibility
> and allowing reasonable error handling for new code), this bug should
> perhaps be targeted to be resolved as part of the GNOME 3 API break.

There's not a general "GNOME 3 API break" plan, and I'm not aware of anyone planning to break any APIs other than dropping the deprecated parts of gtk. Most of the API "break" consists of formally replacing the libraries we've already deprecated (bonobo, gnome-vfs) with the libraries we're already using to replace them (dbus, gio).

Anyway, one non-breaky way to fix this would be to have a SOUP_SERVER_DONT_LISTEN_YET construct-only property. This is lame, because we'd basically be saying "you should always set this property", but it's less lame than being broken I guess :)
Comment 10 Andreas Rottmann 2009-06-08 13:05:44 UTC
(In reply to comment #9)
> 
> Anyway, one non-breaky way to fix this would be to have a
> SOUP_SERVER_DONT_LISTEN_YET construct-only property. This is lame, because we'd
> basically be saying "you should always set this property", but it's less lame
> than being broken I guess :)
> 
That seems like a reasonable solution to me. I'll try my hand at a patch when I come around to it.
Comment 11 Dan Winship 2009-06-08 13:41:26 UTC
Created attachment 136144 [details] [review]
start SoupServer rewrite

Here's some (incomplete) stuff I did a little while back. It looks like
the parts I finished are more focused on the bug 522519 side of the
problem though... you may or may not want to use this as a starting
point.

Also, "SOUP_SERVER_DONT_LISTEN_YET" is an awful name, you should come up
with something better. :)
Comment 12 Dan Winship 2011-07-12 12:31:52 UTC
*** Bug 654449 has been marked as a duplicate of this bug. ***
Comment 13 Dan Winship 2013-06-28 12:37:56 UTC
*** Bug 702911 has been marked as a duplicate of this bug. ***
Comment 14 Dan Winship 2014-05-02 13:27:24 UTC
fixed in git; the existing returning-NULL-from-constructor() API for specifying the listening port/address at construct-time is now deprecated. With the new API, you make calls to soup_server_listen*() after constructing the server, and those functions take a GError** so they can return errors correctly.