Re: [libmicrohttpd] Connection not represented by a file descriptor

[Tomas Heran]

Hi,

thanks for taking the time look at and comment my proposal. Please see
my responses inline ...

Christian Grothoff wrote:
> Hi!
>
> A few things:
>
> 1) I'd avoid the MHD_get_stream_connection_data() entirely and
> instead suggest passing the 'void *' closure instead of
> 'MHD_Connection *' to the callbacks, i.e.:
> ssize_t (*recv_cls_u) (void *cls,
> void *write_to, size_t max_bytes)
> That's one less API call, and it is more in line with our
> usual style. Also, the return value from
> 'MHD_add_stream_connection' could be changed from 'int' to
> 'MHD_Connection *', that way, if the callbacks do need the
> connection, they can just store that handle in the '*cls'.

Got it. I'll look into this.

> 2) I don't see the reason for the 'client_aware' change you
> propose. MHD only calls the completion callback for
> connections that we informed the application about, that's
> simply a question of how the API was defined. Clearly if the
> application never saw the connection, it can't have state to
> clean up about it. Now, in the case of your new extension
> being used, you do want to set 'client_aware' to 'yes'
> immediately (as the client is naturally aware), but I don't
> see a need to change the existing logic.
>
> But maybe I just don't fully get your point.

OK, a little more context on how I use the library is in order here.

Given the specifics of the program I'm using MHD from, I'm doing all
connection management myself and only handing MHD a client connections
once they are "ready to talk" HTTP on (well, I give it one connection
per daemon really).

So, I do roughly this:

1) MHD_start_daemon(... | MHD_USE_NO_LISTEN_SOCKET |
   MHD_USE_STREAM_CONNS ..., MHD_OPTION_NOTIFY_COMPLETED ...)
2) MHD_add_stream_connection
3) Endless loop of MHD_run() which I break only if complete handler
   gets called with non-zero argument. Maybe there's a better way how
   to know when all clients disconnected?

Without my changing the 'client_aware', the complete handler gets called
with 0 once a request is handled (response is sent), but subsequent
client disconnect (which is correctly detected inside the library when
read(...) == 0) does not result in complete handler getting called at
all.

Does this make a little more sense?

> 3) I don't like you passing NULL for the callbacks and then
> later doing an 'if NULL' check. We should just pass
> "recv/send_param_adapter" instead of NULL and avoid
> the branch and never have to worry about NULL.

Makes sense.

> 4) I understand you may not care, but how do you see the
> proposed new API interact with HTTPS? What happens if
> MHD runs with HTTPS and then your API is used? We have
> to at least consider and document those cases.

In our environment/program this is handled at the level of the
connection "object" which is able to encapsulate an SSL/TLS secured
socket as well. But as far as MHD is concerned, it'll simply be using
the supplied the read/write function pointers and the reading/writing
functions, based on the type of the connection (secure or not) would
either use plain read()/write() or SSL_read()/SSL_write().

> 5) The big issue is the poll-ing. Right now, you basically
> say that if the flag for the new option is set, you are
> happy to totally kill performance by doing an extra
> O(n) pass over all connections trying to read/write,
> regardless of whether those are connections from your
> proposed API or traditional sockets.

Agreed that the poll-ing (or rather lack there of) is an issue. As
stated above, my prototype uses one MHD per connection (in a separate
thread), so that's why I did not solve this.

I imagine "poll" callback mechanism would have to be devised - a
way for for MHD to ask the environment it lives in what connections are
readable/writable if any.

I'd definitely look into this some more if we end up working out a
variant of all this that would be acceptable for you.

More on mixing my connections with sockets below ...

> 6) Your patch overall does not give a good answer for how
> this should work with the various event loops; I overall
> dislike extensions that will only work in special cases
> (i.e. only with external select, etc.). I don't see
> how you plan to make that work nicely.

Well, I "ducked" all this by introducing MHD_USE_STREAM_CONNS (sucky
name, I know) flag - when the MHD daemon is started using this, it
assumes it all connections are of this type and will only run
MHD_poll_stream_conns().

I assumed one would only ever want to use one type or another inside
one instance of the MHD daemon.

> 7) Also, what's the point of the 'may_block' arg to
> MHD_poll_stream_conns()? Seems totally dead.

Yes, sorry about that.

> 8) Why do you change the worker pool size from an
> 'unsigned int' to an 'unsigned long long'? I think
> 2^32 threads will still do for quite some time...

Agreed, more than quite some time ;) I believe I made this change based
on a warning generated by lint (on Solaris) ... looking at the code now,
the variable is at one point compared with SIZE_MAX which is 2^64 on
LP64, which was likely the place where the warning was issued. I'll
double check that. But size of the thread pool was never a reason for
me changing this.

> Most importantly, have you really considered _alternatives_? Like say
> just connecting to loopback and feeding the data into the MHD socket --
> say from another thread or a co-routine? I think most of what you try to
> do can be done at the expense of going one more time though TCP/IP (or
> at least a Unix Domain Socket). Still, for most cases that should be
> totally sufficient, and would avoid bloating the API _and_ answer the
> questions of how this is to work with the various event loops.

Actually, a few months back, I did actually try to implement something
like that - precisely because I wanted to avoid introducing those
"object" connections. I had a prototype which almost worked but never
finished it because I really didn't like the complexity (more threads,
unix domain socket, connecting to "myself" etc.) - my concerns were not
as much performance related as they were about the complexity of the
code.

I believe that even when I'll address the poll-ing, then introducing
these object encapsulated connections addition will end up being much
simpler (as in more straightforward) solution than the threaded
looping-back socket "pump" that you propose and I gave a shot and
didn't like ;)

That said, I absolutely understand your concerns about introducing this
to the MHD and bloating the API.

====

I also understand that the way I need to use MHD is quite specific -
basically, I'm not using *any* of the connection management in there
(listening for new connections, handling of the client connections
etc.) and in fact, all I need is an HTTP engine (transport agnostic)
and MHD was the closest (I reviewed a few similar libraries - mostly
the ones you're listing on your website) that I could get this
functionality from with just "a few" changes.

Anyway, I hope this is shedding a little more light on what my goals
and intentions are.

Best regards,
Tomas

> So that's my thoughts right now -- mostly many details to consider and
> answers to find before this would seem to be ready (or, as I said, maybe
> we don't need it if the proposed alternative works).
>
>
> Happy hacking!
>
> Christian
>
>
> On 01/22/2015 10:25 AM, Tomas Heran wrote:
> > Hello,
> >
> > it's been a while and I am sorry for the delay.
> >
> > Please see the attached patch (against 0.9.37) that illustrates what
> > I'm trying to achieve. It basically boils down to allowing the consumer
> > to provide their own recv and send (read_handler and write_handler)
> > functions and an opaque void pointer that represents a connection
> > instead of providing a file descriptor ("pointing" to a socket).
> >
> > The code as is is really useful when there's only one such connection
> > being handled by the MHD daemon (there's no polling implemented yet,
> > but definitely doable).
> >
> > I also understand the patch attached isn't documented (the functions
> > aren't) properly according to the standards of the existing code, so
> > please don't take it as something that I'm asking to be included - it's
> > rather a concrete example showing what I'd like to achieve and what I
> > got to work for my prototype so far and to discuss further.
> >
> > Please let me know what you think.
> >
> > Thanks,
> > Tomas
> >
> > PS: Having gone through the patch again right now, I see that I've also
> > made a change that's related to calling a completion handler (in the
> > situation when client hangs up). The comment explains the issue, I
> > hope. Please let me know whether I should file a separate "bug" for
> > that into bug-tracking. FWIW, all the tests ('make check') pass just
> > fine with this change, but I haven't gone through the testsuite to see
> > whether there's a test that would catch a bug if there was one
> > introduced by such a change. I will if asked to.