GNOME Bugzilla – Bug 710602
soup_server_pause_message() should work before I/O begins
Last modified: 2018-09-21 16:16:43 UTC
If you call soup_server_[un]pause_message() on a SoupMessage which didn’t come from a SoupServerCallback (e.g. one which has just been created using soup_message_new(), you get: (process:28960): libsoup-CRITICAL **: soup_message_io_unpause: assertion 'io != NULL' failed Patch coming up to downgrade the assertion to an if statement.
Created attachment 257814 [details] [review] soup-message-io: Don’t assert on [un]pausing an unsent SoupMessage If a SoupMessage which hasn’t yet been sent is passed into soup_server_[un]pause_message(), an assertion will fail in soup-message-io.c because the message doesn’t yet have any I/O data. Instead of asserting, use an if statement and return silently.
Comment on attachment 257814 [details] [review] soup-message-io: Don’t assert on [un]pausing an unsent SoupMessage um... what are you doing exactly? The test case is invalid. soup_server_pause_message() isn't expected to work on a message that didn't come from the SoupServer. Are you trying to pause a message from a SoupServer::request-started callback? If so, then the fix is that soup_server_pause_message() should be handling that in some way other than by calling soup_message_io_pause(). If you are actually trying to call soup_server_pause_message() on a message that was not generated by the SoupServer, then, No.
(In reply to comment #2) > (From update of attachment 257814 [details] [review]) > um... what are you doing exactly? The test case is invalid. > soup_server_pause_message() isn't expected to work on a message that didn't > come from the SoupServer. The test case isn’t actually what my code does, it’s just a simple way of replicating the crash I saw. I can’t reproduce the crash (and don’t have a backtrace, sadly), which suggests it’s something more subtle. The code I’m using is Aurena’s server: https://github.com/pwithnall/aurena/tree/dlna-playback The soup_server_unpause_message() call is here: https://github.com/pwithnall/aurena/blob/dlna-playback/src/daemon/snra-server-client.c#L611 It operates on a SoupMessage which comes from snra_server_client_new(), which is called here: https://github.com/pwithnall/aurena/blob/dlna-playback/src/daemon/snra-manager.c#L391, which is a callback from soup_server_add_handler(). Tracing through the libsoup code, I can’t find a path which should cause this crash, but it definitely happened. I remember from the trace that the SoupMessage’s private data was pretty much all zeroes, so new_iostream() hadn’t been called on it. > If you are actually trying to call soup_server_pause_message() on a message > that was not generated by the SoupServer, then, No. Sorry this is such a vague bug report; I really should have saved the stack trace when I had it. I guess the best we can do is better document that soup_server_[un]pause_message() should only be called with SoupMessages from a SoupServerCallback.
Created attachment 257895 [details] [review] soup-server: Document restriction on SoupMessages given to SoupServer methods If a SoupMessage which hasn’t yet been sent is passed into soup_server_[un]pause_message(), an assertion will fail in soup-message-io.c because the message doesn’t yet have any I/O data. Document that passing such SoupMessages into those methods is naughty and shouldn’t be done.
Comment on attachment 257895 [details] [review] soup-server: Document restriction on SoupMessages given to SoupServer methods >If a SoupMessage which hasn't yet been sent is passed into >soup_server_[un]pause_message(), an assertion will fail in soup-message-io.c >because the message doesn't yet have any I/O data. It's not "hasn't yet been sent", it's "hasn't yet starting being received". >+ * This must only be called on #SoupMessage<!-- -->s which come from a >+ * #SoupServerCallback added with soup_server_add_handler(). That's not 100% true; you can call it on a message that came from one of the SoupServer signals, as long as you only call it at the right times... Something like "SoupMessages that were created by the SoupServer and are currently doing I/O". Oh, and btw, you don't need to say "#SoupMessage<!-- -->s". gtk-doc has recognized regular plurals for a long time now, so it will linkify "#SoupMessages" correctly.
Created attachment 257987 [details] [review] soup-server: Document restriction on SoupMessages given to SoupServer methods If a SoupMessage which hasn’t yet started to be received is passed into soup_server_[un]pause_message(), an assertion will fail in soup-message-io.c because the message doesn’t yet have any I/O data. Document that passing such SoupMessages into those methods is naughty and shouldn’t be done.
(In reply to comment #5) > That's not 100% true; you can call it on a message that came from one of the > SoupServer signals, as long as you only call it at the right times... This documentation feels icky. It isn’t precise enough to say “are currently doing I/O”, since that requires the user to know when libsoup schedules I/O internally. I really think the functions should be changed to fail gracefully if the SoupMessage isn’t doing I/O. Anyway, here’s a patch which fixes your comments.
committed the documentation patch. updating the bug summary to reflect what the "complete" fix would be
[mass-moving all "UNCONFIRMED" libsoup bugs to "NEW" after disabling the "UNCONFIRMED" status for this product now that bugzilla.gnome.org allows that. bugspam-libsoup-20150210]
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/libsoup/issues/59.