-
Notifications
You must be signed in to change notification settings - Fork 52
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
Fix erroneous exit code not propagating for BatchJob #36
base: master
Are you sure you want to change the base?
Fix erroneous exit code not propagating for BatchJob #36
Conversation
this needs a test covering it |
98a940a
to
54e65c1
Compare
@stof I just added tests. |
$failingClosure = function ($data) use ($expectedExitStatus) { | ||
// Simple condition to simulate only one point of failure | ||
if (3 === $data) { | ||
exit($expectedExitStatus); |
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.
shouldn't we rather have a case throwing an exception in the closure ? This seems a more sensible use case.
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.
Exceptions are indeed more sensible use cases. But this way we cannot test the correctness of the exit status.
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.
@stof I just added a new test case when the closure throws an exception.
54e65c1
to
2d5260d
Compare
@stof Fixed test messages |
$exitStatus = $fork->getExitStatus(); | ||
if (0 !== $exitStatus) { | ||
// Propagate erroneous state | ||
exit($exitStatus); |
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.
The state is indeed propagated but what happens if a child fork throws an exception? Would be very nice to know what exactly went wrong. Or do child forks in that case print the message to stderr?
throw new \InvalidArgumentException($expectedErrorMessage); | ||
} | ||
|
||
exit(0); |
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.
do we absolutely need to exit here ? It would be great if this example could match what people will probably do, i.e. not using exit
at all
@taueres I don't have experience with Spork, so this would need to be reviewed by @kriswallsmith (it is needed anyway as he is the only maintainer) |
Any news? |
@kriswallsmith any new about this pr? thx |
My concern here and the reason I have left it as it is for now is that the user will have no way to know which items in the batch job have processed and which were not. Do you think this a valid concern, or should we say batch jobs must be idempotent so the batch job can simply be run again if something goes wrong? |
@kriswallsmith It does seem like a valid concern. However, I think it's a fair compromise to require idempotence from the jobs. Because at present moment users still can't get any straight-forward information regarding which of the jobs failed. Just my 5¢. |
BatchJob was not correctly propagating erroneous exit codes of its child processes.
If you are looking for bug replication this commit is useful taueres@748dabd (Dockerfile included).