Bug 57070 - VMProcess needs to notifyAll() instead of notify()
Summary: VMProcess needs to notifyAll() instead of notify()
Alias: None
Product: classpath
Classification: Unclassified
Component: classpath (show other bugs)
Version: unspecified
: P3 normal
Target Milestone: 0.99.1
Assignee: Not yet assigned to anyone
Depends on:
Reported: 2013-04-25 16:45 UTC by Carl Ritson
Modified: 2013-07-22 22:03 UTC (History)
1 user (show)

See Also:
Known to work:
Known to fail:
Last reconfirmed:

Test code to reproduce bug (500 bytes, application/octet-stream)
2013-04-25 16:45 UTC, Carl Ritson
Fix for bug (254 bytes, application/octet-stream)
2013-04-25 16:46 UTC, Carl Ritson

Note You need to log in before you can comment on or make changes to this bug.
Description Carl Ritson 2013-04-25 16:45:44 UTC
Created attachment 29940 [details]
Test code to reproduce bug

VMProcess performs a notify() on state change rather than notifyAll(), this means that if one thread waitFor() process termination and another calls destroy() then only one of the two will be woken up.

This occurs during stress testing with Dacapo 2006's Eclipse benchmark on JikesRVM .  Eclipses uses java.lang.Process to execute external commands with a timeout.  Under high load the external processes do not finish within the timeout period and destroy() is called.  At the same time Eclipse's process monitor thread is waiting in waitFor().  Only the waitFor() thread detects termination, the destroy() thread is never resumed.

The attached program can reproduce the bug in miniature.

Interestingly the javadoc suggests that VMProcess should notifyAll() already; however, no versions of the file in CVS or Git appear to have ever performed a notifyAll().
Comment 1 Carl Ritson 2013-04-25 16:46:20 UTC
Created attachment 29941 [details]
Fix for bug
Comment 2 Erik Brangs 2013-06-14 18:32:02 UTC
In case someone views this bug report later: Andrew John Hughes has already committed the fix in revision aaf337541ddfe97dc0ff1cea1f66f06074798a74 (http://git.savannah.gnu.org/cgit/classpath.git/commit/?id=aaf337541ddfe97dc0ff1cea1f66f06074798a74).
Comment 3 Andrew John Hughes 2013-07-22 22:03:53 UTC
I wasn't aware of this bug report.  Now noted: