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

[Christian Grothoff]

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'.

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.

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.

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.

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.

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.

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

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...


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.

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.

signature.asc
Description: OpenPGP digital signature