-
-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: tentative implementation of restart. #135
Conversation
It also better declares dependencies between dtools at_stop tasks.
@@ -1307,7 +1314,8 @@ let () = | |||
(fun p -> | |||
let f = List.assoc "" p in | |||
let wrap_f = fun () -> ignore (Lang.apply ~t:Lang.unit_t f []) in | |||
ignore (Dtools.Init.at_stop wrap_f) ; | |||
(* TODO: this could happen after duppy and other threads are shut down, is that ok? *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't be ok if the function is using things that enqueue stuff. However, enqueueing asynchronous things when shutting down isn't supposed to work anyway..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so, but just wanted to make sure that everybody agrees on that...
Looks fine to me. Let's see what @dbaelde has to say about it.. |
The patch makes sense, but I don't have the details in mind to guess if it's going to work well or not. Did you try to restart the streaming process without relaunching the binary, ie. call the init functions instead of execv at_stop? This might be even faster, and it might be possible to avoid restarting things such as the logs and the server. Also regarding the execv, are you sure that Sys.argv is the right array to pass, compared to Shebang.argv? (One thing to test is the restart when liquidsoap is invoked through a shebang.) @toots, you might have to commit yourself if you want to see Utils.get_some used. Other than that, the new feature is missing documentation. |
The I did not try to reinit everything, but I fear that thing will not go on smoothly since we are carying lots of state (but I will try and report). |
Greenlight here. |
WIP: implementation of restart.
Simple implementation of the restart command for Liquidsoap. What do you think of it?
It also better declares dependencies between dtools at_stop tasks.