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

[Alex Kashchenko]

On 10/04/2018 05:03 PM, Jiri Vanek wrote:
> Thank y ou for review!
> 
> [...]
>
>>>      * rust-launcher/src/os_access.rs: (Os) declared new method 
>>> ofspawn_java_process. (Linux) implementing it by spawn
>>
>> 1. Don't see a reason to use copy/move for its arguments, I'd rather use:
>>
>> jre_dir: &std::path::PathBuf, args: &Vec<String>
> 
> Uf, That would mean, that javadir going in,  will change to path to java 
> bin. That is unexpected. So I kept original

M, this reasoning is not clear, currently you are copying the path on 
the function enter and then immediately copying it second time with the 
clone. This doesn't make sense to me. Not that it is any real difference 
in this case, still there should be some strong reason to NOT pass 
string-like object by const reference.


>
> [...]
>
>> 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.

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 are unclear:

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)?

2. end-user clicked JNLP link in Firefox invoking the launcher - what 
will happen with standard streams in this case?

3. what is the reason to have different handling for stdout and stderr?


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.


>>
>> 4. in error message a space is needed between "," and "\"
> 
> nice catch

This is not actually included in the updated patch (very minor thing of 
course).

>
> [...]
>

[1] 
https://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/invocation.html
[2] 
https://github.com/ojdkbuild/contrib_itw-launcher/blob/75dea38ab870083978675059b7c11d0994172ba9/src/launcher.cpp#L379

>
> [...]
>

-- 
-Alex