Re: [libmicrohttpd] How to close all upgraded connections when shutting down MHD_Daemon?
Hi Nicolas, On 04.05.2017 3:18, Nicolas Mora wrote: > I'm currently working on an implementation of a websocket manager with > MHD and it's getting pretty good so far. > > The problem I have is when the daemon is shut down and there are open > connections. The documentation on MHD_quiesce_daemon says that "Note > that the caller is responsible for closing the returned socket;" which > is close to what I have, but since a websocket can be left open > indefinitely, I need to know if the daemon needs to stop. MHD_quiesce_daemon() prevent MHD from accepting new connections but allow to continue processing with current connections. The returned socket is listening socket. > Is there a signal or an event or any other way for the upgrade_handler > function to know that a MHD_quiesce_daemon or a MHD_stop_daemon has > been sent ? You can track quiesced state of MHD in some global variable (if you are using single MHD instance) or in local variable associated with MHD instance. You will need to update this variable when you quiesced MHD. In upgrade_handler you can check current state of MHD by checking variable. Keep in mind that MHD will not accept new connection after MHD_quiesce_daemon(), so upgrade_handler will be called after MHD_quiesce_daemon() if connection was accepted before MHD_quiesce_daemon() but not yet "upgraded". MHD_stop_daemon() will close all connections including "upgraded" connections. So you can't use "upgraded" connections after MHD_stop_daemon(). Could you explain a bit more situation that you need to solve? Do you want to implement graceful MHD shutdown after closing all upgraded connections? -- Best Wishes, Evgeny Grin
Re: [libmicrohttpd] How to close all upgraded connections when shutting down MHD_Daemon?
Hi, Thanks Evgeny and Silvio for your help Le 2017-05-04 à 03:02, Evgeny Grin a écrit : MHD_quiesce_daemon() prevent MHD from accepting new connections but allow to continue processing with current connections. The returned socket is listening socket. Actually, I'm not using MHD_quiesce_daemon() at all, just MHD_stop_daemon(). I talked about MHD_quiesce_daemon() because in the online documentation, the phrase I quote is below MHD_quiesce_daemon(). You can track quiesced state of MHD in some global variable (if you are using single MHD instance) or in local variable associated with MHD instance. You will need to update this variable when you quiesced MHD. In upgrade_handler you can check current state of MHD by checking variable. Keep in mind that MHD will not accept new connection after MHD_quiesce_daemon(), so upgrade_handler will be called after MHD_quiesce_daemon() if connection was accepted before MHD_quiesce_daemon() but not yet "upgraded". MHD_stop_daemon() will close all connections including "upgraded" connections. So you can't use "upgraded" connections after MHD_stop_daemon(). The problem I have is that MHD_stop_daemon() doesn't close all the connections. Instead, it looks like it waits for the active connections to close by themselves and I have the following log message: Initiated daemon shutdown while "upgraded" connection was not closed. Could you explain a bit more situation that you need to solve? Do you want to implement graceful MHD shutdown after closing all upgraded connections? I guess what I'd rather have is for the upgraded connections to be closed by MHD when MHD_stop_daemon() is called, so the upgrade callback could interpret it easily. Silvio's suggestion might work too, some signal like a "MHD_DAEMON_INFO_WAS_QUIESCED" or, in my case "MHD_DAEMON_INFO_WAS_STOPPED". My current implementation is the following. When a websocket upgrade is called by the client, the websocket callback is executed: https://github.com/babelouest/ulfius/blob/2.0/src/ulfius.c#L432 Then the program loops on listening to the socket https://github.com/babelouest/ulfius/blob/2.0/src/u_websocket.c#L115 But in my example program, when the user stops the program by pressing the key while connections are active, the program doesn't stop and waits for the open websockets to close. https://github.com/babelouest/ulfius/blob/2.0/example_programs/websocket_example/websocket_example.c
Re: [libmicrohttpd] How to close all upgraded connections when shutting down MHD_Daemon?
>> MHD_stop_daemon() will close all connections including "upgraded" >> connections. So you can't use "upgraded" connections after >> MHD_stop_daemon(). >> > The problem I have is that MHD_stop_daemon() doesn't close all the > connections. Instead, it looks like it waits for the active connections > to close by themselves and I have the following log message: > Initiated daemon shutdown while "upgraded" connection was not closed. > Do you use HTTP or HTTPS? You must not call MHD_stop_daemon() while you still have some not closed "upgraded" connections. May be we should add more alarming message in case of such API violation. >> Could you explain a bit more situation that you need to solve? >> Do you want to implement graceful MHD shutdown after closing all >> upgraded connections? >> > I guess what I'd rather have is for the upgraded connections to be > closed by MHD when MHD_stop_daemon() is called, so the upgrade callback > could interpret it easily. > Silvio's suggestion might work too, some signal like a > "MHD_DAEMON_INFO_WAS_QUIESCED" or, in my case > "MHD_DAEMON_INFO_WAS_STOPPED". > > My current implementation is the following. > When a websocket upgrade is called by the client, the websocket callback > is executed: > https://github.com/babelouest/ulfius/blob/2.0/src/ulfius.c#L432 > Then the program loops on listening to the socket > https://github.com/babelouest/ulfius/blob/2.0/src/u_websocket.c#L115 > > But in my example program, when the user stops the program by pressing > the key while connections are active, the program doesn't stop > and waits for the open websockets to close. > https://github.com/babelouest/ulfius/blob/2.0/example_programs/websocket_example/websocket_example.c Could you provide a minimal example? -- Wishes, Evgeny
Re: [libmicrohttpd] How to close all upgraded connections when shutting down MHD_Daemon?
Do you use HTTP or HTTPS? Eventually, since it's a framework for both http and https, I will use both protocols. You must not call MHD_stop_daemon() while you still have some not closed "upgraded" connections. May be we should add more alarming message in case of such API violation. I understand. What I have thought of is to store a signature of all opened upgraded connections in an array, then before calling MHD_stop_daemon(), going through each connection, and close them. I guess that's the most clean way to do it. Could you provide a minimal example? The file https://github.com/babelouest/ulfius/blob/2.0/example_programs/websocket_example/websocket_example.c provides a minimal example of what I intend to do, although it's still in progress.
Re: [libmicrohttpd] How to close all upgraded connections when shutting down MHD_Daemon?
On Thu, May 4, 2017 at 8:47 AM, Nicolas Mora wrote: [...] > Silvio's suggestion might work too, some signal like a > "MHD_DAEMON_INFO_WAS_QUIESCED" or, in my case "MHD_DAEMON_INFO_WAS_STOPPED". I'm going to send a patch to Evgeny ... -- Silvio Clécio
Re: [libmicrohttpd] How to close all upgraded connections when shutting down MHD_Daemon?
On 04.05.2017 16:41, Nicolas Mora wrote: >> >> Do you use HTTP or HTTPS? > Eventually, since it's a framework for both http and https, I will use > both protocols. Behavior in undefined situation can differ in HTTP and HTTPS modes. MHD_stop_daemon() should close all upgraded connections in HTTPS mode. >> You must not call MHD_stop_daemon() while you still have some not closed >> "upgraded" connections. >> May be we should add more alarming message in case of such API violation. > I understand. > What I have thought of is to store a signature of all opened upgraded > connections in an array, then before calling MHD_stop_daemon(), going > through each connection, and close them. I guess that's the most clean > way to do it. Yes, it's most correct way. >> Could you provide a minimal example? >> > The file > https://github.com/babelouest/ulfius/blob/2.0/example_programs/websocket_example/websocket_example.c > provides a minimal example of what I intend to do, although it's still > in progress. >From MHD point of view it's not minimal - a lot of functions and variables that not required to trigger this behavior. -- Wishes, Evgeny
Re: [libmicrohttpd] How to close all upgraded connections when shutting down MHD_Daemon?
Done. So dudes, what do you think about this attached patch? On Thu, May 4, 2017 at 10:45 AM, silvioprog wrote: > On Thu, May 4, 2017 at 8:47 AM, Nicolas Mora > wrote: > [...] > >> Silvio's suggestion might work too, some signal like a >> "MHD_DAEMON_INFO_WAS_QUIESCED" or, in my case "MHD_DAEMON_INFO_WAS_STOPPED". > > > I'm going to send a patch to Evgeny ... > -- Silvio Clécio From 91cec1d7e2a1c69dcf3978e70c4ee12cd9a21513 Mon Sep 17 00:00:00 2001 From: silvioprog Date: Thu, 4 May 2017 10:58:26 -0300 Subject: [PATCH] Allows to check the server shutdown statuses. --- src/include/microhttpd.h | 20 +++- src/microhttpd/daemon.c | 6 ++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/include/microhttpd.h b/src/include/microhttpd.h index ca0059f..fa0b820 100644 --- a/src/include/microhttpd.h +++ b/src/include/microhttpd.h @@ -1841,7 +1841,25 @@ enum MHD_DaemonInfoType * Note: flags may differ from original 'flags' specified for * daemon, especially if #MHD_USE_AUTO was set. */ - MHD_DAEMON_INFO_FLAGS + MHD_DAEMON_INFO_FLAGS, + + /** + * Check if the daemon was shut down. + * No extra arguments should be passed. + */ + MHD_DAEMON_INFO_SHUTDOWN, + + /** + * Check if the daemon was quiesced. + * No extra arguments should be passed. + */ + MHD_DAEMON_INFO_QUIESCED, + + /** + * Check if the daemon has data waiting to be processed. + * No extra arguments should be passed. + */ + MHD_DAEMON_INFO_PENDING_DATA }; diff --git a/src/microhttpd/daemon.c b/src/microhttpd/daemon.c index a91d268..dba818f 100644 --- a/src/microhttpd/daemon.c +++ b/src/microhttpd/daemon.c @@ -6470,6 +6470,12 @@ MHD_get_daemon_info (struct MHD_Daemon *daemon, return (const union MHD_DaemonInfo *) &daemon->connections; case MHD_DAEMON_INFO_FLAGS: return (const union MHD_DaemonInfo *) &daemon->options; +case MHD_DAEMON_INFO_SHUTDOWN: + return (const union MHD_DaemonInfo *) &daemon->shutdown; +case MHD_DAEMON_INFO_QUIESCED: + return (const union MHD_DaemonInfo *) &daemon->was_quiesced; +case MHD_DAEMON_INFO_PENDING_DATA: + return (const union MHD_DaemonInfo *) &daemon->data_already_pending; default: return NULL; }; -- 2.7.4
Re: [libmicrohttpd] How to close all upgraded connections when shutting down MHD_Daemon?
You forgot to update doc/libmicrohttpd.texi, otherwise looks OK even though I'm not convinced SHUTDOWN/QUESTCED are useful: an application can trivially track those itself, so these two are definitively just API bloat. On 05/04/2017 03:59 PM, silvioprog wrote: > Done. So dudes, what do you think about this attached patch? > 0xE29FC3CC.asc Description: application/pgp-keys signature.asc Description: OpenPGP digital signature
Re: [libmicrohttpd] How to close all upgraded connections when shutting down MHD_Daemon?
Same for "pending data". Moreover MHD_DAEMON_INFO_PENDING_DATA is confusing and not correct. Zero in data_already_pending doesn't mean that no data is pending. Zero only means that "no data for immediately process". May be socket is not ready for sending more data, but more data is pending. And mapping "bool" to not bool is not correct. union MHD_DaemonInfo doesn't have "bool" member and currently we do not want to expose "bool" in public header. On some platforms "bool" can be implemented as "char". -- Best Wishes, Evgeny Grin On 04.05.2017 17:20, Christian Grothoff wrote: > You forgot to update doc/libmicrohttpd.texi, otherwise looks OK even > though I'm not convinced SHUTDOWN/QUESTCED are useful: an application > can trivially track those itself, so these two are definitively just API > bloat. > > On 05/04/2017 03:59 PM, silvioprog wrote: >> Done. So dudes, what do you think about this attached patch? >> signature.asc Description: OpenPGP digital signature
[libmicrohttpd] [PATCH] Check response existence on upgrade
When a connection upgrade is requested and when the request sent an error reply, the response is most of the time already sent when the test on connection->response->upgrade_handler is made, leading to dereferencing NULL. Two possibilities exist: NULL == connection->response || NULL == connection->response->upgrade_handler or NULL != connection->response && NULL == connection->response->upgrade_handler The first is prefered because it is probably safer to close the connection in that case. Change-Id: Ie6e7fc165f7fe3635ade0952bb34a0b937d38716 Signed-off-by: José Bollo --- src/microhttpd/connection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/microhttpd/connection.c b/src/microhttpd/connection.c index 4753d6e..91a4492 100644 --- a/src/microhttpd/connection.c +++ b/src/microhttpd/connection.c @@ -882,7 +882,7 @@ keepalive_possible (struct MHD_Connection *connection) #ifdef UPGRADE_SUPPORT if ( (MHD_str_equal_caseless_ (end, "upgrade")) && - (NULL == connection->response->upgrade_handler) ) + (NULL == connection->response || NULL == connection->response->upgrade_handler) ) return MHD_NO; #endif /* UPGRADE_SUPPORT */ -- 2.9.3
Re: [libmicrohttpd] [PATCH] Check response existence on upgrade
Thanks! Applied. -- Best Wishes, Evgeny Grin On 04.05.2017 22:58, José Bollo wrote: > When a connection upgrade is requested and when the > request sent an error reply, the response is most of > the time already sent when the test on > connection->response->upgrade_handler is made, leading > to dereferencing NULL. > > Two possibilities exist: > > NULL == connection->response || NULL == > connection->response->upgrade_handler > > or > > NULL != connection->response && NULL == > connection->response->upgrade_handler > > The first is prefered because it is probably safer to close the > connection in that case. > > Change-Id: Ie6e7fc165f7fe3635ade0952bb34a0b937d38716 > Signed-off-by: José Bollo > --- > src/microhttpd/connection.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/microhttpd/connection.c b/src/microhttpd/connection.c > index 4753d6e..91a4492 100644 > --- a/src/microhttpd/connection.c > +++ b/src/microhttpd/connection.c > @@ -882,7 +882,7 @@ keepalive_possible (struct MHD_Connection > *connection) #ifdef UPGRADE_SUPPORT > if ( (MHD_str_equal_caseless_ (end, > "upgrade")) && > - (NULL == connection->response->upgrade_handler) ) > + (NULL == connection->response || NULL == > connection->response->upgrade_handler) ) >return MHD_NO; > #endif /* UPGRADE_SUPPORT */ > >
