Request for Review: 5049299: (process) Use posix_spawn, not fork, on S10 to avoid swap exhaustion
rob.mckenna at oracle.com
Sat Aug 10 16:16:13 UTC 2013
Thanks for the build review Erik,
Thanks for the review. I'm hoping this webrev has dealt with your comments.
A couple of notes:
- The jexec comments in CompileLauncher referenced the old build too. I
figured it was a handy way to find the corresponding makefile in the old
build. Removed though.
- I've added a return -1 default case to startChild to avoid a "control
reaches end of non-void function" warning on mac & linux. forkAndExec
looks for a negative integer specifically, but this should never
actually be reached.
- Added the ifdef's around spawnChild and its use to avoid an implicit
declaration warning on Linux. (posix_spawn)
I've tested with the attached (quick and very dirty) test. Forgive the
System.gc & the sleep, but as you can see from the output below, it
looks like we're correctly closing our fd's. (a much longer running
version followed the same pattern) Clearly the test is totally
unsuitable for integration, but let me know if you can think of a better
way to do this.
21360 <--- this is the pid
On 28/07/13 22:18, Alan Bateman wrote:
> On 25/07/2013 09:17, Rob McKenna wrote:
>> Thanks a lot Erik,
>> I've added the dependency to the makefile here:
>> Is it ok between the ifeq block?
>> I've altered the launchMechanism code to use valueOf (and lower case
>> names) - much cleaner, thanks for that. I've also limited each
>> platform to supported mechanisms only. With the new layout I don't
>> believe a separate test is needed here. (the default is now
>> posix_spawn/vfork and Basic.java has been altered to run with fork too)
> Thanks Rob, I've taken another pass over the latest webrev and I think
> the finish line is close.
> The launchMechanism determination is much cleaner now (thanks). For
> consistency the enum values should probably be in uppercase and so
> this means you'll need to uppercase the property value to use valueOf.
> It would be nice if launchMechanism were final too (which you do by
> having the privileged action be of type LaunchMechanism rather than Void).
> The comment in UNIXProcess.java.bsd references vfork/exec which I
> assume is copied from the Linux version and should be removed.
> Did you consider having forkAndExec take the helper as a parameter?
> Just wonder if this would be cleaner than having to use JNI to grab
> the field value.
> It's usually best to use sprintnf rather than sprintf (in spawnChild)
> to avoid any concerns (or static analysis tools) that wonder about
> buffer overflow.
> Style nit in the C code is that many places have spurious space
> between the function name and the parentheses.
> A pre-existing bug (only noticed because it has moved from
> UNIXProcess_md.c to childproc.c) is that close is not restartable.
> The comment in in makefiles/CompileLauncher.gmk references the old
> build, is that needed?
> So overall I don't see any issues, it would be really good to stress
> this to make sure that we aren't leaking file descriptors. I don't
> know if we have an existing test that would help with that.
> One final comment is that the new files seems to have the pure GPL
> comment, I assume you will add the GPL+ Classpath Exception comment
> before you push this.
More information about the core-libs-dev