[rfc][icedtea-web] spawning linux jvm in rust lunchers

Tue Oct 9 13:06:25 UTC 2018

On 10/8/18 8:47 PM, Alex Kashchenko wrote:
> On 10/08/2018 06:15 PM, Jiri Vanek wrote:
>>
>> [...]
>> >>>> 3. please setup std in/out/err handling explicitly (even if you want
>>>>> to inherit all of them)
>>>>
>>>> Well, done, but why?
>>>
>>> I think standard streams.handling on process spawn are important enough to be explicit about them.
>>
>> Well, from my view, to much early, but sure.
>>>
>>> The choice to spawn JVM in a separate process (instead of running it in the same process with 
>>> Invocation API [1]) looks like a logical choice to me. But other details about the spawned process 
>> I'm not sure with it. In shell scripts we are using exec, but it donot need to persist.
>> I would be happy for wider discussion on this topic.
>>
>>> are unclear:All questions are rightful, and I do not have much answers.
>>>
>>> 1. what is an intended lifetime of the launcher process after JVM is spawned (it currently 
>>> exits,  > should it linger longer to do something with the std streams)?
>>
>>  From this view, better would be to continue ITW in this same process. Then this decission fade away.
> 
> It is not hard to implement in-process launching in rust without additional libs (small unsafe block 

Not hard x reliable and commonly used are two different things. As I have not found solution, I
think we are stuck in  spawn for now ...
> with dlopen and JNI_CreateJavaVM). But launching out-of-process allows to easily re-launch ITW from 
> command line with exactly the same environment (windows launcher currently logs the full java 
> invocation string for that). So I'd rather stay with out-of-process spawning.
... which is not so bad after all.

> 
> If needed, launcher process can wait on the child with waitpid [1] and do some housekeeping 
> before/after the waiting. As I understand, std::process::Command can do exactly that with status() [2].

I think this should be the behaviour. Adapted patch attahed
> 
> 
>> Currenly its only purpose can be some stds observation, but it is unlikely rightful.
>> It should die, and all lunching logs shoudl be closed. Any other logging should be in java 
>> competence.
> 
> So current behaviour (exit after spawn) is the one currently intended.

Tbh, I dont know how inherited streams  if parent dies. AFAIK they persists, but maybe likely it can
depend on environment and individual impelmentations?
> 
> 
>>> 2. end-user clicked JNLP link in Firefox invoking the launcher - what will happen with standard 
>>> streams in this case?
>>
>> I have seen both - it coudl inherited firefox's streams,  or create its own.
>> As I consider terminal as rightfull part of the ITW, they should notbe lost.
> 
> I am not sure that we should touch browser stdstreams. Current behaviour looks correct for command 
> line launch though.
> 

I really think it should stay on defaults. If browser is starting it as child, then stream  should
be fully inherited.
I like the headless mode  of ITW, and it works if process is started as child of (most) browsers.
The best will be the tee-ing of the streams.  Maybe configurable, maybe with different defualts on
linux/windows.

> Going forward, I suggest to handle std streams in "GUI launch" and "CLI launch" cases differently - 
> to have some launcher-specific flag ignored by ITW to pass the GUI/CLI mode to launcher or something 
> like that.

Yes. This idea will be somehow included.
> 
> 
>>> 3. what is the reason to have different handling for stdout and stderr?
>>
>> None? Do you see it somewhere?
>> Stdout shoudl go to inherited stdout, and stderr to inherited stder.. And stdin form inherrited 
>> stdin.
> 
> I meant this part "stderr to inherited stder", it may be better to redirect stderr to stdout 
> (without requiring user to add 2>&1), but with "launch ITW as with bash" reasoning - current logic 
> is fine.

Ugh. Why would you do that? Stderr and stdout have theirs purposes. I would never intentionally
mingle them.
> 
> 
>>> Note, that Windows ITW launcher in ojdkbuild drops stdin and redirects stdout and stderr into a 
>>> file [2]. This is probably the wrong choice for cross-platform launcher (you likely need stdin 
>>> sometimes), but some conscious decision should be done about std streams.
>>
>> Linux one must keep it. And the luncher should be robust enough, to work both with it and without 
>> it, and in parallel allowing for both file ends.  So in ideal word, the stdour+err will bee 
>> tee-ed-like.
> 
> Maybe to have both GUI and CLI "launch modes" supported, but to have CLI by default in Linux and GUI 
> by default in Windows.
> 
> 
>>
>> [...]
>>
>> Do you have some idea how to do the exec-like behaviour in rust?
> 
> Assuming exec(1) [3] here (not exec(3) [4]), current behaviour (args passed though, stdstreams 
> inherited) looks close enough to exec to me. Additionally we can also wait for ITW to exit with 
> status() [2] and pass its exit code back to shell.

Only close enough. The " replaces the current process image with a new process image." is not
achieved - pid changes.
Both exec(1) and exec(3) do the same thing at the end.
> 
> 
>> Attached is updated patch, but I think the spawn x exec-like should be decided now, and should be 
>> incorporated.
> 
> Patch looks good to me, I guess "await for ITW to exit" and more stdstreams handling can be added 
> later.

I included the  "await for ITW to exit" as it seems to affect the trait.

I'm wondering, if
https://doc.rust-lang.org/std/process/struct.Command.html#method.status
do the same, as
https://doc.rust-lang.org/std/process/struct.Child.html#method.wait
(https://doc.rust-lang.org/src/std/process.rs.html#761-764)

> 
> 
>>
>> [...]
>>
> [1] https://linux.die.net/man/3/waitpid
> [2] https://doc.rust-lang.org/std/process/struct.Command.html#method.status
> [3] https://linux.die.net/man/1/exec
> [4] https://linux.die.net/man/3/exec
> 
> 

Thanx a lot!

-- 
Jiri Vanek
Senior QE engineer, OpenJDK QE lead, Mgr.
Red Hat Czech
jvanek at redhat.com    M: +420775390109
-------------- next part --------------
A non-text attachment was scrubbed...
Name: spawning4.patch
Type: text/x-patch
Size: 2546 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20181009/888f816f/spawning4.patch>