Skip to content

Fix platform type inititilization#6431

Merged
headius merged 1 commit intojruby:masterfrom
MariuszCwikla:bug_in_initPlatform
Oct 9, 2020
Merged

Fix platform type inititilization#6431
headius merged 1 commit intojruby:masterfrom
MariuszCwikla:bug_in_initPlatform

Conversation

@MariuszCwikla
Copy link
Contributor

@MariuszCwikla MariuszCwikla commented Oct 8, 2020 •
edited
Loading

Accidentally I found a bug in Platform.initPlatform(). On Windows it returns UnixPlatform even though it has following logic:

    private static Platform initPlatform(){
        try {
            if (IS_WINDOWS)
                return new NTPlatform();

It turns out that ordering of static variables is matters.
When debugging it turns out that Platform.OS is null and IS_WINDOWS is not correctly computed.

Simplest solution is to just move INSTANCE below OS and IS_WINDOWS.
Tested on Oracle JDK 8 and Open JDK 11.

@headius
Copy link
Member

headius commented Oct 9, 2020

Wow, this is a great find! I am amazed it did not break anything in the handful of Windows CI tests we run.

@headius headius added this to the JRuby 9.3.0.0 milestone Oct 9, 2020
@headius headius merged commit 2d12159 into jruby:master Oct 9, 2020
@headius
Copy link
Member

headius commented Oct 9, 2020

@enebo @kares Perhaps this should go into 9.2.14 as well? I guess we do not know of any bugs caused by it, so I'm not sure how to weigh the small risk involved.

@kares
Copy link
Member

kares commented Oct 10, 2020 •
edited
Loading

@headius this is not that "risky", .java parts only use the Platform.* constants - there's only one place where the instance is retrieved -> Process.groups will be broken on Windows (and Solaris) but it's not-implemented in Windows anyways.

@MariuszCwikla MariuszCwikla deleted the bug_in_initPlatform branch October 11, 2020 16:52
@headius
Copy link
Member

headius commented Oct 12, 2020

@kares Yeah at a glance the exposure looked pretty small, and now it seems largely non-existent. Probably better to get it into 9.2 line just in case some future patch exposes this issue.

@headius
Copy link
Member

headius commented Oct 12, 2020

Cherrypicked to master in cce44fe.

@kares
Copy link
Member

kares commented Oct 13, 2020

using Process.groups will likely go heavy on warnings (due relying on JDK internals).
I'll take a stub at using POSIX-y groups instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

JRuby 9.3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@MariuszCwikla @headius @kares