Fix race condition in test harness #1173
Conversation
| while ( ! feof($this->pipes[2]) ) { | ||
| $errorMessage .= fgets($this->pipes[2]); | ||
| } | ||
| if ($exitStatus !== 0) { |
There was a problem hiding this comment.
I don't know how accurate this is, but the proc_close documentation states that:
Returns the termination status of the process that was run. In case of an error then -1 is returned.
I don't know if it's possible that a value other than 0 or -1 is returned, but elsewhere I've written code that explicitly compares against -1.
There was a problem hiding this comment.
exit status can be a range of values not just 0 or -1. The unit tests have a 127 and a 1 return values (this can be seen in the logs).
[edit to add]
.. of the unit tests
2019-12-02 20:27:56 [error] Error 2 executing external filter process: sh: 1: Syntax error: "(" unexpected
2019-12-02 20:27:56 [error] Error 127 executing external filter process: sh: 1: gobbledygook: not found
| while (($buffer = fgets($this->pipes[2])) !== false) { | ||
| $errorOutput .= $buffer; | ||
| } | ||
| fclose($this->pipes[2]); |
There was a problem hiding this comment.
Is it possible that anything additional could be written to STDOUT and STDERR between here and the proc_close below? I'm curious if this can be put after the proc_close, but I've never tried that.
There was a problem hiding this comment.
The docs state that you should close all open file handles before the proc_close to avoid deadlocks. Since STDERR is read until EOF (or error) then you shouldn't get any more data out of stderr. Any data sitting in the stdout buffer will get thrown away, but this should be handled by the code starting on line 121.
If you add a filter that writes an infinite amount of data to stderr after stdin is closed then the proc_close() call will never execute. Don't do this.
The unit tests were not passing in my docker. The reason was because there is no guarantee that the process has exited by the time the
proc_get_status()function is called. So the test would fail if the process was still in the running state when the status function was executed.This change removes the
proc_get_status()call and instead uses the return value from the proc_close which is guaranteed to give a yes/no answer.