Closed sevein closed 6 years ago
The transition from a threaded app to single-threaded looks good too - is this something we will want for all deployments of MCPClient going forward or just something for Jisc? I wonder if maybe we should be retaining the threaded version for other deployment scenarios? For example when there isn't a watchdog to ensure the mcpclient service(s) is still running.
That's actually a very good idea. If I add the single-threaded version as a new executable, e.g. archivematicaSingleClient.py
, then we'll be able to upstream this work to vanilla in a backward-compatible manner. I'll do that, it shouldn't be hard. Thanks for the idea.
My other concern is that we're removing a lib file from archivematicaCommon, because it's no longer used in archivematicaClient.py. Is this only place where the detectCores was being used? If so then fine, we can resurrect from history if it's needed again, otherwise it should be left in.
I don't delete code unless I confirm it's dead code but with your suggestion if the old entry point remains I'll also have to restore that module.
Thank you so much!
I felt very tempted to write this in Go by the way. It's funny that I've spent all this time writing web applications in Python but when it comes to concurrency topics I find Go much easier because of its channels or things like context
or select
.
I guess it's never too late to learn more about multi-threaded programming and the synchronization mechanisms provided by the threading
module in Python - I'm a total illiterate at that! One of my first attempts was to update the multi-threaded version so it stops accepting new jobs as long as the signal is received but also wait up to 30 secs for the ongoing jobs to complete before killing them. I also wanted to signal the subprocesses so they could also try to exit gracefully. I tried a few things but I failed badly. Multi-threaded programming is extremely difficult!
@lower29 I've just submitted a new commit where I add a new entry point instead of making the changes in the original one. The diff is now bigger because I moved things around a bit to avoid duplication of code. It's probably easier if you look at the tree directly instead of deciphering the diff.
I've re-reviewed @sevein's changes and have run a test transfer through the new MCP client code to confirm it works. Happy to approve 👍
RDSSARK-374
This pull request introduces a simplified version of MCPClient where threaded workers are replaced by a single worker loop in the main thread. This loop is now forced to exit by a signal handler
quit
whenSIGINT
orSIGTERM
signals are received.This implementation forces the worker to exit immediately which is potentially better than having ECS killing it after the 30 seconds timeout is exceeded. This causes the running jobs to stop and Gearman will eventually schedule them to the next available worker.