graceful shutdown #178
graceful shutdown #178
Conversation
…n a shutdown_delay is provided
| @@ -38,6 +38,9 @@ client_fun(Socket, [{send_pid, To} | Cmds]) -> | |||
| To ! {client, self()}, | |||
| client_fun(Socket, Cmds); | |||
| client_fun(Socket, [{send, Data, Tester} | Cmds]) -> | |||
| client_fun(Socket, [{send, Data, Tester, 0} | Cmds]); | |||
| client_fun(Socket, [{send, Data, Tester, Delay} | Cmds]) -> | |||
| timer:sleep(Delay), | |||
There was a problem hiding this comment.
It's not good to have tests that sleep, they can cause the test runs to take much longer than they should and they can intermittently pass or fail depending on external factors (e.g. the specs or load on the system running the tests). It's best to have code that doesn't require the wall clock to work, or at least use mocking when testing it to avoid any sleeping.
| close_listen_socket(State), | ||
| State1 = State#mochiweb_socket_server{shutdown_notify_pid=From, acceptor_pool_size=0}, | ||
| % Reply will be given when active_socket count goes to 0 | ||
| {noreply, State1}; |
There was a problem hiding this comment.
Removed the polling logic by using a noreply response and a reply when active_sockets become 0.
|
I have made another commit to incorporate the changes you mentioned @etrepum - Removing of the polling logic from the source and the timer:sleep in the unit test. Please can you review the changes and merge to master? thanks. |
| @@ -60,12 +61,15 @@ set(Name, Property, _Value) -> | |||
| [Name, Property]). | |||
|
|
|||
| stop(Name) when is_atom(Name) orelse is_pid(Name) -> | |||
| gen_server:call(Name, stop); | |||
| gen_server:call(Name, stop); | |||
| stop({Scope, Name}) when Scope =:= local orelse Scope =:= global -> | ||
| stop(Name); | ||
| stop(Options) -> | ||
| State = parse_options(Options), | ||
| stop(State#mochiweb_socket_server.name). | ||
| stop(Name, Timeout) when is_atom(Name) orelse is_pid(Name) andalso is_integer(Timeout) -> |
There was a problem hiding this comment.
andalso has precedence over orelse. Therefore, this guard will be true for:
is_atom(Name) (independently of Timeout's type)
or
is_pid(Name) andalso is_integer(Timeout)
Is that the expected behaviour? I believe it should be:
| stop(Name, Timeout) when is_atom(Name) orelse is_pid(Name) andalso is_integer(Timeout) -> | |
| stop(Name, Timeout) when (is_atom(Name) orelse is_pid(Name)) andalso is_integer(Timeout) -> |
This change is to allow for a graceful shutdown of the Mochiweb server.