Page MenuHomePhabricator

Fix ProcessIO test failures
ClosedPublic

Authored by labath on Mar 11 2015, 4:12 AM.

Details

Summary

There was a race condition regarding the output of the inferior process. The reading of the
output is performed on a separate thread, and there was no guarantee that the output will get
eventually consumed. Because of that, it was happening that calling Process::GetSTDOUT was not
returning anything even though the process was terminated and would definitely not produce any
further output. This was usually happening only under very heavy system load, but it can be
reproduced by placing an usleep in the stdio thread (Process::STDIOReadThreadBytesReceived).

This patch addresses this by adding synchronization capabilities to the Communication thread.
After calling Communication::SynchronizeWithReadThread one can be sure that all pending input has
been processed by the read thread. This function is then called after every public event which
stops the process to obtain the entire process output.

Diff Detail

Event Timeline

labath updated this revision to Diff 21685.Mar 11 2015, 4:12 AM
labath retitled this revision from to Fix ProcessIO test failures.
labath updated this object.
labath edited the test plan for this revision. (Show Details)
labath added reviewers: clayborg, jingham.
labath added a subscriber: Unknown Object (MLST).
jingham edited edge metadata.Mar 11 2015, 9:47 AM

Shouldn't there be implementations of InterruptRead in this patch?

It seems like you are saying InterruptRead will consume all available stdout, then return Interrupted, which is a little odd for an interrupt. But I don't know what InterruptRead actually does...

Jim

InterruptRead is already implemented, here I have only added pure virtual method to the superclass, so they can be accessed easily. For implementations, refer to source/Host/posix/ConnectionFileDescriptorPosix.cpp and source/Host/windows/ConnectionGenericFileWindows.cpp.

It seems like you are saying InterruptRead will consume all available stdout, then return Interrupted, which is a little odd for an interrupt.

Yes that is basically what I am saying. Maybe "interrupt" is not exactly the best name for this, but it was already present, so I used it. In the general case, this is exactly what InterruptRead will do though - make the thread blocked on Read() return immediately with eConnectionStatusInterrupted. In this comment, I have just tried to explain what happens in the edge case, when there is input readily available *and* you call InterruptRead() - here, the priority is given to reading the input.

clayborg accepted this revision.Mar 11 2015, 10:44 AM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Mar 11 2015, 10:44 AM
This revision was automatically updated to reflect the committed changes.