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.
