Re: [libmicrohttpd] suspend/resume patch

[Flavio Ceolin]

Hi folks,

Any other thoughts about this ? Should I consider this change not wanted
?

> 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");

Regards,

Flavio Ceolin
Intel Open source Technology Center