Re: [libmicrohttpd] [PATCH]: MHD_connection_update_event_loop_info sends INTERNAL_ERROR for suspended connections
On 17.03.2017 5:42, Vitaliy T wrote: > Inside of MHD_connection_handle_read () src/microhttpd/connection.c is > missing check against of the return value for the > try_grow_read_buffer() call. It's just ignored. And now without a fear > I may say that the INTERNAL_ERROR will be send for a connection that > was resumed and then suspended again (probably few times). > > Here is the log of the server: > > +0300 2017-03-17 05:31:34.254320 * Connection from 127.0.0.1 port > 47824 [33/49728] > +0300 2017-03-17 05:31:34.261671 * Connection from 127.0.0.1 port 48848 > +0300 2017-03-17 05:31:34.272435 ! 127.0.0.1 port 47824: suspend > +0300 2017-03-17 05:31:34.279515 ! 127.0.0.1 port 46800: uploaded `big2' > +0300 2017-03-17 05:31:34.280380 ! 127.0.0.1 port 47824: resumed > +0300 2017-03-17 05:31:34.280917 ! 127.0.0.1 port 48848: uploading `big3' > +0300 2017-03-17 05:31:34.281012 * Connection 127.0.0.1 port 46800 closed: OK > does not fit #2: > pool: 0x0x14206d50 > pool->pos: 66565 > asize: 67376 > pool->end: 130976 > FAIL: try_grow_read_buffer > FAIL: MHD_connection_handle_read > 127.0.0.1 port 47824 > +0300 2017-03-17 05:31:34.281732 ! 127.0.0.1 port 47824: suspend > does not fit #2: > pool: 0x0x14206d50 > pool->pos: 66565 > asize: 67376 > pool->end: 130976 > FAIL: try_grow_read_buffer > ERROR: 127.0.0.1 port 47824 > Error processing request (HTTP response code is 500 > (`Internal server error tle>Some programmer needs to study the manual more > carefully.')). Closi > ng connection. > +0300 2017-03-17 05:31:34.378595 ! 127.0.0.1 port 48848: uploaded `big3' > +0300 2017-03-17 05:31:34.384555 ! 127.0.0.1 port 47824: resumed > +0300 2017-03-17 05:31:34.384716 * Connection 127.0.0.1 port 48848 closed: OK > +0300 2017-03-17 05:31:34.384857 * Connection 127.0.0.1 port 47824 closed: OK > > As we can see the connection 127.0.0.1 port 47824 was suspended, > resumed and then again suspended. > > The server was run in epoll mode with 1 thread. > DEFAULT_HTTPD_CONNECTION_MEMORY_LIMIT = 128k > DEFAULT_HTTPD_CONNECTION_MEMORY_INCREMENT = 4k > Both suspend & resume operations are called from DH. > > Hope, it will help. To get the full picture, could you also monitor value pointed by 'upload_data_size' before and after your MHD_AccessHandlerCallback is called? Please note that even if you suspend connection, you must process at least some data (preferably - all data) and decrement value of 'upload_data_size'. -- Best Wishes, Evgeny Grin
Re: [libmicrohttpd] [PATCH]: MHD_connection_update_event_loop_info sends INTERNAL_ERROR for suspended connections
On 17.03.2017 10:03, Evgeny Grin wrote: > On 17.03.2017 5:42, Vitaliy T wrote: >> As we can see the connection 127.0.0.1 port 47824 was suspended, >> resumed and then again suspended. >> >> The server was run in epoll mode with 1 thread. >> DEFAULT_HTTPD_CONNECTION_MEMORY_LIMIT = 128k >> DEFAULT_HTTPD_CONNECTION_MEMORY_INCREMENT = 4k >> Both suspend & resume operations are called from DH. >> >> Hope, it will help. > > To get the full picture, could you also monitor value pointed by > 'upload_data_size' before and after your MHD_AccessHandlerCallback is > called? > > Please note that even if you suspend connection, you must process at > least some data (preferably - all data) and decrement value of > 'upload_data_size'. Processing logic was updated: while connection is suspended overflown buffer will not cause connection to fail (as it will not generate busy-waiting for suspended connection), but once connection is resumed - connection will fail if application will not read data from buffer in callback. -- Best Wishes, Evgeny Grin
Re: [libmicrohttpd] [PATCH]: MHD_connection_update_event_loop_info sends INTERNAL_ERROR for suspended connections
Hello, On 17 March 2017 at 10:03, Evgeny Grin wrote: > Please note that even if you suspend connection, you must process at > least some data (preferably - all data) and decrement value of > 'upload_data_size'. Yes, this was my mistake. Before suspending a connection I have to forgot about decrementing upload_data_size. I have questions: 1) Due the async nature of the MHD, how can I keep a connection in resume state _without_ reading data in DH? I mean, I can't say in which moment I can resume the connection with guarantee to read the data. I have to create some kind of queue with tracking which connection is could read data and which ones could not. And more importantly that I must guarantee that if I resumed a connection and it must read full data. Things getting complicated. Current DH logic looks like that: 1. I have to initialize internal structure per request, then it will be stored to con_cls and I return MHD_YES. 2. On the second entry into DH, I do process request: either upload a file, either suspend a connection. 3. When uploading the file has been finished I am resuming the next connection (if it is). 4. Even after resuming the connection there is a chance that a new connection will be proceeded before the "resumed" connection. I can track if the connection was resumed and give it a higher priority. But if so I have to suspend _all_ new connections before doing something useful. I will think about it, but, IMHO, it is wrong that I can't suspend/resume connections when I want without reading buffers. 2) Is it possible to do so that the chain suspend-resume-suspend-resume will not affect read buffers? Until I will "say" to MHD that I really read a data and we can continue in the normal mode. Because, by you words I can't resume a connection if I don't know will be it suspended again or not. I can resume the connection only if I give guarantees that it will be proceeded as usual (read all data). Thanks. -- With Best Regards, Vitaliy V. Tokarev
Re: [libmicrohttpd] [PATCH]: MHD_connection_update_event_loop_info sends INTERNAL_ERROR for suspended connections
On 17.03.2017 15:10, Vitaliy T wrote: > Hello, > > On 17 March 2017 at 10:03, Evgeny Grin wrote: >> Please note that even if you suspend connection, you must process at >> least some data (preferably - all data) and decrement value of >> 'upload_data_size'. > > Yes, this was my mistake. Before suspending a connection I have to > forgot about decrementing upload_data_size. > > I have questions: > > 1) Due the async nature of the MHD, how can I keep a connection in > resume state _without_ reading data in DH? > > I mean, I can't say in which moment I can resume the connection with > guarantee to read the data. I have to create some kind of queue with > tracking which connection is could read data and which ones could not. > And more importantly that I must guarantee that if I resumed a > connection and it must read full data. > > Things getting complicated. > > Current DH logic looks like that: > > 1. I have to initialize internal structure per request, then it will > be stored to con_cls and I return MHD_YES. > 2. On the second entry into DH, I do process request: either upload a > file, either suspend a connection. > 3. When uploading the file has been finished I am resuming the next > connection (if it is). > 4. Even after resuming the connection there is a chance that a new > connection will be proceeded before the "resumed" connection. > > I can track if the connection was resumed and give it a higher > priority. But if so I have to suspend _all_ new connections before > doing something useful. > I will think about it, but, IMHO, it is wrong that I can't > suspend/resume connections when I want without reading buffers. > > 2) Is it possible to do so that the chain > suspend-resume-suspend-resume will not affect read buffers? Until I > will "say" to MHD that I really read a data and we can continue in the > normal mode. Because, by you words I can't resume a connection if I > don't know will be it suspended again or not. I can resume the > connection only if I give guarantees that it will be proceeded as > usual (read all data). Looks like you want to blindly resume connection and check whether to process it only in callback handler. I suggest to move check for "readiness to be processed" to some separate function. Then instead of blindly resuming connection and immediately suspending it again - resume connection only when you will be sure that it's ready to be processed. You don't need to make this check only inside callback function. If you suspend connection and resume it later - connection conditions are not changed, you will not get any new information in callback. So you can store required information in your internal structures and check it *before* resuming, not *after* resuming. Resume/suspend repeated cycles without processing of information - is ineffective and wastes a lot of resources. -- Best Wishes, Evgeny Grin PS If you will check master on official git, you may find support of your ineffective way of processing.
Re: [libmicrohttpd] [PATCH]: MHD_connection_update_event_loop_info sends INTERNAL_ERROR for suspended connections
On 17 March 2017 at 15:43, Evgeny Grin wrote: > You don't need to make this check only inside callback function. If you > suspend connection and resume it later - connection conditions are not > changed, you will not get any new information in callback. So you can > store required information in your internal structures and check it > *before* resuming, not *after* resuming. Seems to be I got the point. Thanks for the advice. > Resume/suspend repeated cycles without processing of information - is > ineffective and wastes a lot of resources. There is a little problem. If I would proceed the data and right after that I suspend the connection, then I must keep this (read) data somewhere, because a new data already is arrived. This leads to next problem: more suspended connections means that the app. will keep even more data (one proceeded, one in MHD buffers). IMHO, 1) the application becomes overcomplicated; 2) there is even more wasting of resources. Either, I chose the wrong tool :) > PS If you will check master on official git, you may find support of > your ineffective way of processing. Yes, I have tested the patch. As I suggested early, it's works flawlessly. It is harmless, even if you say that is "wrong way" :) Ok, lets finish at this point, I must finish my project. Later, I will give you a feedback. Thanks again! -- With Best Regards, Vitaliy V. Tokarev
Re: [libmicrohttpd] [PATCH]: MHD_connection_update_event_loop_info sends INTERNAL_ERROR for suspended connections
On 17.03.2017 15:59, Vitaliy T wrote:
> On 17 March 2017 at 15:43, Evgeny Grin wrote:
>> You don't need to make this check only inside callback function. If you
>> suspend connection and resume it later - connection conditions are not
>> changed, you will not get any new information in callback. So you can
>> store required information in your internal structures and check it
>> *before* resuming, not *after* resuming.
>
> Seems to be I got the point. Thanks for the advice.
>
>> Resume/suspend repeated cycles without processing of information - is
>> ineffective and wastes a lot of resources.
>
> There is a little problem. If I would proceed the data and right after
> that I suspend the connection, then I must keep this (read) data
> somewhere, because a new data already is arrived. This leads to next
> problem: more suspended connections means that the app. will keep even
> more data (one proceeded, one in MHD buffers). IMHO, 1) the
> application becomes overcomplicated; 2) there is even more wasting of
> resources.
You are storing connection handler somewhere anyway.
Declare:
struct my_conn_info
{
const MHD_connection * conn;
enum req_method {req_UNK, req_GET, req_POST, req_HEAD, req_OTHER} method;
const char * url;
unsigned int timeout;
int other_data1;
char other_data2;
} my_connection
and use it instead of MHD connection handler for you internal functions.
Pretty simple and take only a few more bytes per connection.
It's really much lighter comparing to calling heavy systems call like
send/recv/select.
And you can use it with callbacks as well (as con_cls). Anyway you need
to store connection specific data while you incrementally processing
incoming data.
>
> Either, I chose the wrong tool :)
This is a free software. You always have a choice. :)
>> PS If you will check master on official git, you may find support of
>> your ineffective way of processing.
>
> Yes, I have tested the patch. As I suggested early, it's works
> flawlessly. It is harmless, even if you say that is "wrong way" :)
> Ok, lets finish at this point, I must finish my project. Later, I will
> give you a feedback.
>
Good to know that it's working.
Send more reports! And thanks for that report.
--
Best Wishes,
Evgeny Grin
Re: [libmicrohttpd] Websockets
Websockets and RFC6455. I noticed the test_upgrade.c unit test code doesn't provide the accept handshake key combination hashing specified on page 8 of the RFC, in the section "opening handshake." Apparently we're supposed to combine the websocket key with a predesignated static GUID, hash it, and send it back. Without doing this, current firefox refuses to accept a websocket connection to MHD. My question is, should I write this functionality myself or are there plans for MHD to implement it in the future with macros or such? It seems very easy to implement but I don't want to implement my own code if the library has plans to implement this functionality for users directly. Don't want to duplicate efforts etc. Thanks; ~JM On Mon, Mar 13, 2017 at 3:51 PM, Christian Grothoff wrote: > On 03/13/2017 09:39 PM, John Duncan wrote: > > Quick question. I noticed when I built the library, it didn't move > > mhd_sockets.h into the includes. That file contains all the > > MHD_send_/MHD_recv_ macros used in the websockets unit test. Am I > supposed > > to re-implement the functions/macros found in there, or am I supposed to > > include that code with my project? I know I could just snip out the code > > and use it, but for forward compatibility I'd like to know, from the > devs, > > how I should proceed for the least number of future-headaches. > > Those are just convenience wrappers for us around send()/recv(). You are > not expected to use those at all in your own code. The upgrade method > gives you a socket and applications can expect to be able to use the > usual system calls (read/write/send/recv, etc.) directly with the > (web)socket. > > Just be aware that you might have been given a socketpair() in case > there's actually HTTPS underneath. Thus, stuff like ioctl() and > setsockopt() may not work correctly. If anyone needs support there, > this would have to be added to MHD: This is what MHD_upgrade_action() is > supposed to be used for in the future, in addition to the current > close()-logic. > > Happy hacking! > > Christian > >
Re: [libmicrohttpd] Websockets
On 03/18/2017 01:44 AM, John Duncan wrote: > Websockets and RFC6455. I noticed the test_upgrade.c unit test code > doesn't provide the accept handshake key combination hashing specified on > page 8 of the RFC, in the section "opening handshake." > > Apparently we're supposed to combine the websocket key with a predesignated > static GUID, hash it, and send it back. Without doing this, current > firefox refuses to accept a websocket connection to MHD. > > > My question is, should I write this functionality myself or are there plans > for MHD to implement it in the future with macros or such? It seems very > easy to implement but I don't want to implement my own code if the library > has plans to implement this functionality for users directly. Don't want > to duplicate efforts etc. Right now, the expectation is that you need to write this yourself, at least I have no plans to do more in terms of API than what we have today with respect to "HTTP Upgrade". Happy hacking! Christian signature.asc Description: OpenPGP digital signature
Re: [libmicrohttpd] Websockets
No problem. Thank you so much for your work on the library. It's great! When I write some macros to do what's required I'll follow them up with a mailing list post so others can use them if they want. Shouldn't take too long to do. ~J On Fri, Mar 17, 2017 at 2:55 PM, Christian Grothoff wrote: > On 03/18/2017 01:44 AM, John Duncan wrote: > > Websockets and RFC6455. I noticed the test_upgrade.c unit test code > > doesn't provide the accept handshake key combination hashing specified on > > page 8 of the RFC, in the section "opening handshake." > > > > Apparently we're supposed to combine the websocket key with a > predesignated > > static GUID, hash it, and send it back. Without doing this, current > > firefox refuses to accept a websocket connection to MHD. > > > > > > My question is, should I write this functionality myself or are there > plans > > for MHD to implement it in the future with macros or such? It seems very > > easy to implement but I don't want to implement my own code if the > library > > has plans to implement this functionality for users directly. Don't want > > to duplicate efforts etc. > > Right now, the expectation is that you need to write this yourself, at > least I have no plans to do more in terms of API than what we have today > with respect to "HTTP Upgrade". > > Happy hacking! > > Christian > >
Re: [libmicrohttpd] Websockets
Hello John, I'm sure this example will be very useful for us! :-) Thank you! On Fri, Mar 17, 2017 at 9:59 PM, John Duncan wrote: > No problem. Thank you so much for your work on the library. It's great! > When I write some macros to do what's required I'll follow them up with a > mailing list post so others can use them if they want. Shouldn't take too > long to do. > > ~J > > On Fri, Mar 17, 2017 at 2:55 PM, Christian Grothoff > wrote: > >> On 03/18/2017 01:44 AM, John Duncan wrote: >> > Websockets and RFC6455. I noticed the test_upgrade.c unit test code >> > doesn't provide the accept handshake key combination hashing specified >> on >> > page 8 of the RFC, in the section "opening handshake." >> > >> > Apparently we're supposed to combine the websocket key with a >> predesignated >> > static GUID, hash it, and send it back. Without doing this, current >> > firefox refuses to accept a websocket connection to MHD. >> > >> > >> > My question is, should I write this functionality myself or are there >> plans >> > for MHD to implement it in the future with macros or such? It seems >> very >> > easy to implement but I don't want to implement my own code if the >> library >> > has plans to implement this functionality for users directly. Don't >> want >> > to duplicate efforts etc. >> >> Right now, the expectation is that you need to write this yourself, at >> least I have no plans to do more in terms of API than what we have today >> with respect to "HTTP Upgrade". >> >> Happy hacking! >> >> Christian >> > -- Silvio Clécio
Re: [libmicrohttpd] Websockets
Are you the same Silvio from the Defcon wolves among us talk? If so, you're a god. On Fri, Mar 17, 2017 at 3:05 PM, silvioprog wrote: > Hello John, > > I'm sure this example will be very useful for us! :-) > > Thank you! > > On Fri, Mar 17, 2017 at 9:59 PM, John Duncan > wrote: > >> No problem. Thank you so much for your work on the library. It's >> great! When I write some macros to do what's required I'll follow them up >> with a mailing list post so others can use them if they want. Shouldn't >> take too long to do. >> >> ~J >> >> On Fri, Mar 17, 2017 at 2:55 PM, Christian Grothoff >> wrote: >> >>> On 03/18/2017 01:44 AM, John Duncan wrote: >>> > Websockets and RFC6455. I noticed the test_upgrade.c unit test code >>> > doesn't provide the accept handshake key combination hashing specified >>> on >>> > page 8 of the RFC, in the section "opening handshake." >>> > >>> > Apparently we're supposed to combine the websocket key with a >>> predesignated >>> > static GUID, hash it, and send it back. Without doing this, current >>> > firefox refuses to accept a websocket connection to MHD. >>> > >>> > >>> > My question is, should I write this functionality myself or are there >>> plans >>> > for MHD to implement it in the future with macros or such? It seems >>> very >>> > easy to implement but I don't want to implement my own code if the >>> library >>> > has plans to implement this functionality for users directly. Don't >>> want >>> > to duplicate efforts etc. >>> >>> Right now, the expectation is that you need to write this yourself, at >>> least I have no plans to do more in terms of API than what we have today >>> with respect to "HTTP Upgrade". >>> >>> Happy hacking! >>> >>> Christian >>> >> > -- > Silvio Clécio >
Re: [libmicrohttpd] Websockets
Nevermind, was thinking of Silvio Cesare. He used to go by silvioprogramming back in the 90s is why I asked. Sorry for the mixup! On Fri, Mar 17, 2017 at 3:05 PM, silvioprog wrote: > Hello John, > > I'm sure this example will be very useful for us! :-) > > Thank you! > > On Fri, Mar 17, 2017 at 9:59 PM, John Duncan > wrote: > >> No problem. Thank you so much for your work on the library. It's >> great! When I write some macros to do what's required I'll follow them up >> with a mailing list post so others can use them if they want. Shouldn't >> take too long to do. >> >> ~J >> >> On Fri, Mar 17, 2017 at 2:55 PM, Christian Grothoff >> wrote: >> >>> On 03/18/2017 01:44 AM, John Duncan wrote: >>> > Websockets and RFC6455. I noticed the test_upgrade.c unit test code >>> > doesn't provide the accept handshake key combination hashing specified >>> on >>> > page 8 of the RFC, in the section "opening handshake." >>> > >>> > Apparently we're supposed to combine the websocket key with a >>> predesignated >>> > static GUID, hash it, and send it back. Without doing this, current >>> > firefox refuses to accept a websocket connection to MHD. >>> > >>> > >>> > My question is, should I write this functionality myself or are there >>> plans >>> > for MHD to implement it in the future with macros or such? It seems >>> very >>> > easy to implement but I don't want to implement my own code if the >>> library >>> > has plans to implement this functionality for users directly. Don't >>> want >>> > to duplicate efforts etc. >>> >>> Right now, the expectation is that you need to write this yourself, at >>> least I have no plans to do more in terms of API than what we have today >>> with respect to "HTTP Upgrade". >>> >>> Happy hacking! >>> >>> Christian >>> >> > -- > Silvio Clécio >
Re: [libmicrohttpd] Websockets
Hehe :-D :-D On Fri, Mar 17, 2017 at 11:29 PM, John Duncan wrote: > Nevermind, was thinking of Silvio Cesare. He used to go by > silvioprogramming back in the 90s is why I asked. Sorry for the mixup! > > On Fri, Mar 17, 2017 at 3:05 PM, silvioprog wrote: > >> Hello John, >> >> I'm sure this example will be very useful for us! :-) >> >> Thank you! >> >> On Fri, Mar 17, 2017 at 9:59 PM, John Duncan >> wrote: >> >>> No problem. Thank you so much for your work on the library. It's >>> great! When I write some macros to do what's required I'll follow them up >>> with a mailing list post so others can use them if they want. Shouldn't >>> take too long to do. >>> >>> ~J >>> >>> On Fri, Mar 17, 2017 at 2:55 PM, Christian Grothoff >> > wrote: >>> On 03/18/2017 01:44 AM, John Duncan wrote: > Websockets and RFC6455. I noticed the test_upgrade.c unit test code > doesn't provide the accept handshake key combination hashing specified on > page 8 of the RFC, in the section "opening handshake." > > Apparently we're supposed to combine the websocket key with a predesignated > static GUID, hash it, and send it back. Without doing this, current > firefox refuses to accept a websocket connection to MHD. > > > My question is, should I write this functionality myself or are there plans > for MHD to implement it in the future with macros or such? It seems very > easy to implement but I don't want to implement my own code if the library > has plans to implement this functionality for users directly. Don't want > to duplicate efforts etc. Right now, the expectation is that you need to write this yourself, at least I have no plans to do more in terms of API than what we have today with respect to "HTTP Upgrade". Happy hacking! Christian >>> >> -- >> Silvio Clécio >> > -- Silvio Clécio
