Re: [libmicrohttpd] suspend/resume patch

[Flavio Ceolin]

Hi Christian,

> Hi Flavio,
> 
> Well, if we do bother to detect this, my preference would be not to
> ignore the bad behavior, but to literally call the panic function (i.e.
> abort()).  Suspending an already suspended connection or resuming one
> that isn't already resumed is indicative of a serious logic error in the
> application.
> 
> Yes, we could return an error code, but that'll just result in lots
> of applications doing (assert(OK == MHD_suspend/resume/...())) which
> isn't really any better.
> 
> My 2 cents
> 
> Christian

Thanks for the fast reply. I agree that is a logic error in the
application, but I do not see this as an error (from the microhttpd
perspective), I mean, is quite usual this kind of functions just return
if there is nothing to do.

My usage is a kind of wrapper for libmicrohttpd, in my case, the
responses are always asynchronous. When a request comes I suspend the
connection and give the request to an user callback, then the user call
a send_response() wherever he wants and in this point a resume the
connection. To do this, I need to keep the connection status in my
handle to avoid resume more than one and also to resume before stop the
daemon.

We can live with this of course, but i think this will become the usage
simpler and this will not be a performance problem because this is not a
critical function, probably is called few times in an application.

> 
> On 04/14/2016 07:38 PM, Flavio Ceolin wrote:
> > Hi folks,
> > 
> > Is there any reason for microhttpd do not check if a connections is
> > already resumed or suspended and handle this, instead of have undefined
> > behavior ? As far as I could see, the connection struct already has the
> > suspended field, so it's just a simple check.
> > 
> > For the user perspective is quite annoying and prone to error have to
> > maintain the connection status in its side. Bellow the patch, any
> > suggestion/explanation is welcome.
> > 
> > 
> > Index: src/include/microhttpd.h
> > ===================================================================
> > --- src/include/microhttpd.h        (revision 37040)
> > +++ src/include/microhttpd.h        (working copy)
> > @@ -1957,10 +1957,7 @@
> >  
> >  /**
> >   * Resume handling of network data for suspended connection.  It is
> > - * safe to resume a suspended connection at any time.  Calling this
> > - * function on a connection that was not previously suspended will
> > - * result in undefined behavior.
> > - *
> > + * safe to resume a suspended connection at any time.
> >   * @param connection the connection to resume
> >   */
> >  _MHD_EXTERN void
> > Index: src/microhttpd/daemon.c
> > ===================================================================
> > --- src/microhttpd/daemon.c (revision 37040)
> > +++ src/microhttpd/daemon.c (working copy)
> > @@ -1714,6 +1714,8 @@
> >  {
> >    struct MHD_Daemon *daemon;
> >  
> > +  if (connection->suspended)
> > +    return;
> >    daemon = connection->daemon;
> >    if (MHD_USE_SUSPEND_RESUME != (daemon->options & MHD_USE_SUSPEND_RESUME))
> >      MHD_PANIC ("Cannot suspend connections without enabling 
> > MHD_USE_SUSPEND_RESUME!\n");
> > @@ -1781,6 +1783,9 @@
> >  {
> >    struct MHD_Daemon *daemon;
> >  
> > +  if (!connection->suspended)
> > +    return;
> > +
> >    daemon = connection->daemon;
> >    if (MHD_USE_SUSPEND_RESUME != (daemon->options & MHD_USE_SUSPEND_RESUME))
> >      MHD_PANIC ("Cannot resume connections without enabling 
> > MHD_USE_SUSPEND_RESUME!\n");
> > 
> 




-- 
Flavio Ceolin
Intel Open source Technology Center