Asynchronous forked workers with specs #220
Conversation
To achieve that, all logging actions are moved out to the parent process, so child process only does the workload and marshals it back to parent process. Forked workers should NOT re-use parent connection (sanity check added). The code makes use of exit!(0) command, that triggers fast exit from a forked process, so postgres does not close the forked connection. Adds specs for forked workers success/failure, mixed workers, connection reuse exception.
…nt process and log completion with a newly established connection
|
I refactored some stuff to be more pretty. Also merged fix for the spec that always fails (connection count). Booya! |
| @pid = Process.pid | ||
| end | ||
|
|
||
| def needs_its_own_connection? |
There was a problem hiding this comment.
Not sure the method name is right here. Shouldn't it be something like: new_fork?, forked?, just_forked? or something like that? Name is not clear enough until we read the actually code.
|
I did not review the tests yet, gotta move on other stuff. I'll wait on feedback before continuing :) |
Add Worker#kill_child hook Add ConnAdapter#establish, #handle_fork Add more comments regarding piping Update comments to say that exit!(0) is a successful exit
|
Thank you @jipiboily for the meaningful comments! I updated PR to address all of them. The comments regarding exit!(0) were unclear. That is an explicit way to exit the child process with successful status. So no zombie connections, I added more test. And regarding the new block parameter in #work method, it is called within a forked process after the job is done. Tests use it to pass execution results back to main thread (as it allows now to do that manually) for assertions. |
|
Ugh. I learned a thing or two about determinism in tests. I removed some assertions that caused race conditions. |
|
This has been stuck for a long time now. I think we should probably close it? 😕 Thoughts @senny ? |
|
@jipiboily I'm not using forking workers. However if they are actually broken that isn't good. We should either fix it or remove broken functionality. Doesn't help to keep it around if it doesn't work and we are not fixing it. |
|
Closing, it's super old and not finished. @Inviz, lmk if you wanna re-open - I'd happily help you out. |
This is a follow up PR to #218. Sorry that it has got merged, but I don't see a better way to do without access to repo (which I don't really need).
It may be not rock solid, but I wrote specs and they pass.