View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002984 | libmicrohttpd | external API | public | 2013-08-10 11:47 | 2013-08-13 14:36 |
| Reporter | bplant | Assigned To | Christian Grothoff | ||
| Priority | normal | Severity | feature | Reproducibility | always |
| Status | closed | Resolution | won't fix | ||
| Product Version | 0.9.27 | ||||
| Target Version | 0.9.29 | Fixed in Version | 0.9.29 | ||
| Summary | 0002984: Can't queue connections when using MHD_add_connection | ||||
| Description | If a connection is attempted to be added using MHD_add_connection but daemon->max_connections <= 0, MHD_NO is returned. This would be ok, except that is no way to distinguish this from another failure in MHD_add_connection (such as a malloc failure for example). Is it possible to change the return value when daemon->max_connections <= 0? My max connections is very low because I'm running on a microprocessor that only has a few hundred KB of RAM. | ||||
| Tags | No tags attached. | ||||
|
|
That cannot really be changed in a backwards-compatible fashion. However, we might be able to do something with 'errno'. I would suspect it is already set "correctly" for out-of-memory (ENOMEM), and we might set 'errno' to something like 'ENFILE' if we have too many connections. Would that do? |
|
|
Should be fixed in SVN 28486 (using errno). |
|
|
I don't think the socket should be closed (when using MHD_add_connection) before errno is set and MHD_NO is returned as this doesn't allow me to try adding the connection again (when hopefully some other connections have been processed and it gets accepted). |
|
|
If you really want to do that, you could 'dup' the socket first. I think the possibility of a socket leak because we return an error code and the client fails to check is too real to change this. Not to mention that we'd again run into the issue of not being backwards compatible, this time in a really bad way (sudden socket leak!). So really, if you need this, just 'dup' first, then you can close the duplicate FD later if the MHD_add_connection succeeded. |
|
|
Unfortunately the "operating system" (if you can really call it that) doesn't support dup(). I wouldn't have thought it would be too much of an issue if the socket wasn't closed ONLY when passed via MHD_add_connection. Yes, it would break backwards compatibility, but I'm probably the only person using this feature ;-) |
|
|
Sadly, you're mistaken: I know other projects that also _do_ use that API. And I'd _really_ dislike bloating the API with an 'add2' over this. The other option, adding another flag is also extremely ugly. Other ideas? |
|
|
Wow, I never thought anyone else would use this feature when I requested it 2 or so years back! Unfortunately I cannot think of another way of doing it. In my mind, the "correct" way (if there is such a thing) is to not close the socket. Not because this is going to suit me/my project better, but because MHD isn't "responsible" for this socket. I.e. MHD didn't create the socket and therefore I don't think it should be allowed to close it. This is obviously in contrast to the sockets that MHD creates itself when a new request hits port 80 and thus *should* close. On the other hand; I really dislike changing APIs. As for the 'add2' function and bloating, it wouldn't add too many bytes to the compiled code as there are only 2 lines of code in the current MHD_add_connection function. But I also dislike having multiple functions that do *almost* the same thing. Where does it end? What to do, what to do...... |
|
|
Also, each exported symbol increases code size and linking/loading costs. Now, I still think that if you give the socket to MHD, you give up control --- after all, depending on what happens MHD may close it anyway shortly thereafter. Here is another idea: what if we added a way for you to 'reserve' a certain number of connection slots for (external) "MHD_add_connection" operations? That way, you might be able to do counting (how many did YOU add before explicitly that were not yet finished) to pre-determine the outcome, despite "concurrent" accept operations going on. |
|
|
A very good point - the socket is also closed after a HTTP request is completed successfully. I guess the difference here though is that the request was completed successfully so there would never be any need to retry. Not sure about the counting. I'd have to use a callback when the request is completed, but how would I distinguish between normal connections and those that I add using MHD_add_connection? My connection limit is currently set to 5. If I increase it to 10, it works, but that's in one particular browser. Different browsers behave differently and might make more parallel requests. No matter what I increase the limit to, there's always the possibility of a connection being aborted because the limit is reached without some for of queuing mechanism available. |
|
|
Possibly a little bit naughty, but what about a #ifdef to determine if the socket should be closed or not? It wouldn't break backwards compatibility (they wouldn't have the option defined), it wouldn't create an extra API function and it resolves my issue :-) |
|
|
>> How would I distinguish between normal connections and those that I add using MHD_add_connection? << -- MHD_get_connection_info can be used to get the socket (and then you can check if the number matches). And if you know that out of 10 connections, you reserved 1 for MHD_add_connection and that one is in use, you know you must wait to queue another unless you're willing to risk to fail. The bigger issue is that if the server is idle (0 of the other 9 are in use), you'd still only be able to queue one connection via MHD_add_connection. |
|
|
That's probably going to vary depending on the use case. My use case is a web interface to a piece of hardware. It's very unlikely that there will be more than 2 people accessing the web interface concurrently. Therefore in a perfect world the whole 10 (or 5 as it was originally set to) connection slots could be used by either traditional connections or those added via MHD_add_connection. |
|
|
Right, but if you were to divide them up into two groups, say 5+5, would that be OK? |
|
|
It would work provided that the counting was accurate. Is it as simple as decrementing the "in-use count" when MHD_RequestCompletedCallback() fires and incrementing it whenever MHD_add_connection returns MHD_YES? |
|
|
Yes, it _should_ be that simple. |
|
|
Here's another (big) hack: use a negative FD. We could add a rule that says that if the FD is negative, MHD_add_connection must not close it on error. So that's like a one-bit option. (Naturally, if fd < 0, we'd use -fd for the file descriptor internally.). This fails if your FD might be zero AND you need this feature, but for 99.9999% of all users FD 0 is stdin, and if stdin was closed, it should be the listen socket, so the cases where the socket passed to MHD_add_connection is ZERO and you must not close it (because the server is busy on all connections) seems pretty extreme to the point of not being relevant. What do you think? |
|
|
What if there was an api call that returned the number of available connection slots? |
|
|
With threads, whatever that API returns might be invalid the next instruction (race). |
|
|
Thanks for all the suggestions. I'm wondering it perhaps it would be easier if I just modified the MHD source code myself to suit my purposes. I don't like to do this as these sorts of changes often get forgotten about which cause things to break when libraries are upgraded down the track, but I've only got 256KB of program memory to play with and I'm already having to trim other stuff down to get things to fit, so I can't really justify adding code to lock/unlock a mutex to maintain a counter when I can instead comment out one line of code. |
|
|
I agree, given that you're having some very special use-case on a very constraint system, maybe this is the best solution. I understand that this is not perfect, but at least your 'diff' to the code is tiny. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2013-08-10 11:47 | bplant | New Issue | |
| 2013-08-10 20:32 | Christian Grothoff | Note Added: 0007327 | |
| 2013-08-10 20:41 | Christian Grothoff | Note Added: 0007328 | |
| 2013-08-10 20:41 | Christian Grothoff | Status | new => resolved |
| 2013-08-10 20:41 | Christian Grothoff | Fixed in Version | => 0.9.29 |
| 2013-08-10 20:41 | Christian Grothoff | Resolution | open => fixed |
| 2013-08-10 20:41 | Christian Grothoff | Assigned To | => Christian Grothoff |
| 2013-08-10 20:42 | Christian Grothoff | Target Version | => 0.9.29 |
| 2013-08-11 23:25 | bplant | Note Added: 0007337 | |
| 2013-08-11 23:25 | bplant | Status | resolved => feedback |
| 2013-08-11 23:25 | bplant | Resolution | fixed => reopened |
| 2013-08-11 23:40 | Christian Grothoff | Note Added: 0007341 | |
| 2013-08-12 00:03 | bplant | Note Added: 0007342 | |
| 2013-08-12 00:03 | bplant | Status | feedback => assigned |
| 2013-08-12 00:42 | Christian Grothoff | Note Added: 0007343 | |
| 2013-08-12 11:18 | bplant | Note Added: 0007345 | |
| 2013-08-12 11:23 | Christian Grothoff | Note Added: 0007346 | |
| 2013-08-12 12:19 | bplant | Note Added: 0007347 | |
| 2013-08-12 13:18 | bplant | Note Added: 0007348 | |
| 2013-08-12 13:52 | Christian Grothoff | Note Added: 0007349 | |
| 2013-08-12 14:19 | bplant | Note Added: 0007350 | |
| 2013-08-12 14:53 | Christian Grothoff | Note Added: 0007351 | |
| 2013-08-12 15:02 | bplant | Note Added: 0007352 | |
| 2013-08-12 15:04 | Christian Grothoff | Note Added: 0007353 | |
| 2013-08-12 23:59 | Christian Grothoff | Note Added: 0007361 | |
| 2013-08-13 00:28 | bplant | Note Added: 0007362 | |
| 2013-08-13 08:57 | Christian Grothoff | Note Added: 0007363 | |
| 2013-08-13 14:13 | bplant | Note Added: 0007365 | |
| 2013-08-13 14:36 | Christian Grothoff | Note Added: 0007366 | |
| 2013-08-13 14:36 | Christian Grothoff | Severity | major => feature |
| 2013-08-13 14:36 | Christian Grothoff | Status | assigned => closed |
| 2013-08-13 14:36 | Christian Grothoff | Resolution | reopened => won't fix |
| 2024-01-21 13:24 | Christian Grothoff | Category | libmicrohttpd API => external API |
