Partial fix #2923: Improve javalib 64 bit UnixProcess waitFor handling #2972
Conversation
There was a problem hiding this comment.
Looks good, thank you for your work in improving work with processes.
Overall new algorithm looks good to me, I belive you have far better knowledge of working with poll events then I do. I only have few suggestions for working with Options to make code a bit more readable and idiomatic.
| val ev = | ||
| if (_exitValue.isEmpty) "not exited" | ||
| else _exitValue.head.toString() |
There was a problem hiding this comment.
How about using a fold here for a cleaner code?
| val ev = | |
| if (_exitValue.isEmpty) "not exited" | |
| else _exitValue.head.toString() | |
| val ev = _exitValue.fold("not exited")(_.toString()) |
There was a problem hiding this comment.
Done. Thank you for the improvement.
There was a problem hiding this comment.
Overall new algorithm looks good to me, I belive you have far better knowledge of working with poll events then I do
Thank you for the encouragement, especially coming from the person who re-wrote the unix Process and waitFor for
Windows.
| if (_exitValue.isDefined) { // avoid wait-after-wait complexity | ||
| _exitValue.head | ||
| } else { // wait until process exits or forever, whichever comes first. | ||
| osWaitForImpl(None).getOrElse(1) // 1 == EXIT_FAILURE, unknown cause | ||
| } |
There was a problem hiding this comment.
| if (_exitValue.isDefined) { // avoid wait-after-wait complexity | |
| _exitValue.head | |
| } else { // wait until process exits or forever, whichever comes first. | |
| osWaitForImpl(None).getOrElse(1) // 1 == EXIT_FAILURE, unknown cause | |
| } | |
| _exitValue // avoid wait-after-wait complexity | |
| .orElse(osWaitForImpl(None)) // wait until process exits or forever, whichever comes first. | |
| .getOrElse(1) // 1 == EXIT_FAILURE, unknown cause |
| if (_exitValue.isDefined) { // avoid wait-after-wait complexity | ||
| true | ||
| } else { | ||
| val tv = stackalloc[timespec]() | ||
| fillTimeval(timeout, unit, tv) | ||
|
|
||
| // wait until process exits or times out. | ||
| osWaitForImpl(Some(tv)).isDefined | ||
| } |
There was a problem hiding this comment.
| if (_exitValue.isDefined) { // avoid wait-after-wait complexity | |
| true | |
| } else { | |
| val tv = stackalloc[timespec]() | |
| fillTimeval(timeout, unit, tv) | |
| // wait until process exits or times out. | |
| osWaitForImpl(Some(tv)).isDefined | |
| } | |
| _exitValue // avoid wait-after-wait complexity | |
| .orElse { | |
| // wait until process exits or times out. | |
| val tv = stackalloc[timespec]() | |
| fillTimeval(timeout, unit, tv) | |
| osWaitForImpl(Some(tv)) | |
| }.isDefined |
LeeTibbert
force-pushed
the
PR_j_l_UnixProcess_I2923
branch
from
fb004bd
to
24dde94
Compare
LeeTibbert
added
the
backport candidate
|
Thank you for the merge. After this PR has proved itself in the 0.5.0 stream for a while (weeks?), I believe |
The goal of this PR is to reduce the defects which ProcessTest is validly reporting in Continuous Integration (CI).
This has been seen particularly often on macOS.
It is marked "Partial fix" because many hundreds of CI runs need to be monitored to see if it has the desired
effect. It is hard to prove a negative.
This PR has benefited from conversations with Arman Bilge and study of his
armanbilge/epollcatrepository,particularly the
kevent64handling. Merit may go to Arman; I retain ownership of all bugs & defects.This PR makes changes in javalib for 64 bit systems running either Linux >= 5.3 or macOS.
All 32 bit systems, older Linux systems, and systems running other operating systems
continue to use the now legacy
UnixProcess/UnixProcessGen1code.When/if it proves itself, it could probably be extended to FreeBSD and other BSD derivative.
Reviewers will note that the new
UnixProcesGen2code does not use the C++ProcessMonitorcode.This PR provides the basis for a second PR which will modify
UnixProcessGen2to useposix_spawn().That method is faster than
fork()and a step on the road to using an even faster upcominguring_spawn().Pass the hot sauce please.