Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initialization timing issue for static variable #1671

Merged
merged 2 commits into from
Jul 9, 2021

Conversation

KyongSik-Yoon
Copy link
Contributor

currentPlatform variable initialized in static block of this class.
But it's also accessed via constructor for validate unsupported environment.
This variable possible not initialized yet when access make a instance use constructor.
I'm already met this kind of issue when I run unit test use this library.
I think one more variable init checking required, at least If constructor want to check unsupported or not.

currentPlatform variable initialized in static block of this class.
But it's also accessed via constructor for validate unsupported environment.
This variable possible not initialized yet when access make a instance use constructor.
I'm already met this kind of issue when I run unit test use this library.
I think one more variable init checking required, at least If constructor want to check unsupported or not.
@coveralls
Copy link

coveralls commented Jun 25, 2021

Coverage Status

Coverage remained the same at 85.764% when pulling 3bb54b7 on KyongSik-Yoon:master into 7364882 on oshi:master.

@dbwiddis
Copy link
Member

It is my understanding that the order of operations in Java executes static variable initialization first (in source code order) followed by instance variable initialization, and only after that, the constructor. So I don't see where there's a problem with the existing code.

Your change introduces lazy instantiation which is known to be not thread safe, so the suggestion as-is makes things worse. If lazy instantiation were needed using the Memoizer class would be preferred.

But I don't see a need to do that for a static final variable. Am I mistaken?

@dbwiddis dbwiddis requested a review from mprins June 25, 2021 15:50
@KyongSik-Yoon
Copy link
Contributor Author

KyongSik-Yoon commented Jun 28, 2021

@dbwiddis It's occured only our CI process in gitlab.
When I run single unit test. There is no problem.
If possible I will take thread dump in this case.

I'm using 5.2.3 version. And this problem appeared after upgrade to latest version.
In this situation error message displayed like below.
Operating system not supported: JNA Platform type
When I logging getCurrentPlatform() result is UNKNOWN.
A difference with 5.2.3 and lastest version is checking for unsupported platform in consturctor.
So I assumed invalid state of currentPlatform variable.

@mprins
Copy link
Member

mprins commented Jun 28, 2021

Odd problem, can you show us your code?

The JNA stuff is pure java and according to the java spec any acces to a static class member will be preceded by execution of the static initializers unless a class instance has aready been created in which case the static initilizers are executed before the constructor.
So I think the existing code is correct.

@dbwiddis
Copy link
Member

The error message "Operating system not supported: JNA Platform type" should have been followed by a number.

The currentPlatform value you note was returned as UNKNOWN is a final variable and cannot change. So it was initialized once as UNKNOWN based entirely on calls to JNA's Platform class. The result is based on static final variables in that class as well.

Please check that you have the latest version of JNA (and JNA-platform) as a dependency, although that shouldn't matter here If you are using a supported operating system, then the bug is in JNA properly identifying it, which it does with JVM properties. Can you answer a few more questions:

  • What number is reported in the exception after "JNA Platform type [this value]"
  • What operating system are you using?
  • What JDK are you using (vendor and version)?
  • What is the contents of System.getProperty("os.name") ?

@dbwiddis dbwiddis marked this pull request as draft June 28, 2021 21:55
@KyongSik-Yoon
Copy link
Contributor Author

KyongSik-Yoon commented Jun 30, 2021

@dbwiddis What number is reported in the exception after "JNA Platform type [this value]"
==> Mesage was displayed with number 2. At that time currentPlatform is UNKNOWN

What operating system are you using?
==> Windows 10

What JDK are you using (vendor and version)?
==> openjdk version "1.8.0_292"
OpenJDK Runtime Environment (build 1.8.0_292-b10)
OpenJDK 64-Bit Server VM (build 25.292-b10, mixed mode)

What is the contents of System.getProperty("os.name") ?
==> "Windows 10"

@KyongSik-Yoon
Copy link
Contributor Author

@mprins Here is my code.

class StorageOnSystemSpec : FunSpec({
    test("display as string") {
        println(StorageOnSystem(SystemInfo()).diskAndPartition())
    }
})

Error occured when call a constructor SystemInfo()

@dbwiddis
Copy link
Member

displayed with number 2. At that time currentPlatform is UNKNOWN

Well, this is very interesting because 2 is indeed Windows.

From JNA's Platform class (as expected):

    public static final int WINDOWS = 2;

    private static final int osType;

    static {
        String osName = System.getProperty("os.name");
        if (osName.startsWith("Linux")) {
    ...
        }
        else if (osName.startsWith("Windows")) {
            osType = WINDOWS;
        }

and

    public static final boolean isWindows() {
        return osType == WINDOWS || osType == WINDOWSCE;
    }

called by OSHI

    private static final PlatformEnum currentPlatform;

    static {
        if (Platform.isWindows()) {
            currentPlatform = WINDOWS;
        } else if (Platform.isLinux()) {
    ...
    }

So the Platform.isWindows() call is apparently returning false during OSHI initialization, but would be true if called later.

It looks like you're calling this from Kotlin code. I wonder if there's some subtle difference there.

I'm baffled as to how this can occur.

@dbwiddis
Copy link
Member

Could you point us to the CI log for the case where this is failing?

@dbwiddis
Copy link
Member

This StackOverflow post may be related. Or not.

@dbwiddis
Copy link
Member

dbwiddis commented Jul 2, 2021

It has been suggested that using a static method rather than a static initialization block may fix this.

Please update this PR to do something like:

private static final PlatformEnum currentPlatform = queryCurrentPlatform();

and then define

private static PlatformEnum queryCurrentPlatform() {
   // code from the static initialization block, but return instead of setting variable value
}

@KyongSik-Yoon
Copy link
Contributor Author

KyongSik-Yoon commented Jul 4, 2021

ok, I will do that.

@KyongSik-Yoon
Copy link
Contributor Author

I updated this PR.
I will comment whether it is effect to me or not.

@dbwiddis dbwiddis marked this pull request as ready for review July 9, 2021 15:37
@dbwiddis dbwiddis merged commit 455185c into oshi:master Jul 9, 2021
@dbwiddis
Copy link
Member

dbwiddis commented Jul 9, 2021

Thanks! I went ahead and merged this. It should be in SNAPSHOT shortly to aid your testing. I may tweak the class ordering later (to move the method below the constructor) but that shouldn't change the behavior.

KyongSik-Yoon pushed a commit to KyongSik-Yoon/oshi that referenced this pull request Jul 15, 2021
oshi#1671 not worked for me.
Run CI with snapshot version but I saw same error message below.
```
[ERROR] display as string  Time elapsed: 0.016 s  <<< ERROR!
java.lang.UnsupportedOperationException: Operating system not supported: JNA Platform type 2
Caused by: java.lang.UnsupportedOperationException: Operating system not supported: JNA Platform type 2
```
JNA could recognize current os in static block when failed on a constructor of SystemInfo class.
So I tried check unknown environment by JNA method instead of OSHI. At the result my CI passed successfully.
I'm not sure what is exact reason. But I just think that JNA and OSHI depends on many static initialization and it's make not easy to assume proper working.

In my opinion, static blocks and variables should be minimized.
But if OSHI have important reason to check unsupported environment in constructor this MR need to approved.

There is remained riddle why OSHI's static variable and JNA static method result don't have consistency.
Anyway, This change working for me.
Please consider enough.
dbwiddis added a commit that referenced this pull request Jul 18, 2021
…1680)

* Initialization timing issue for static variable

currentPlatform variable initialized in static block of this class.
But it's also accessed via constructor for validate unsupported environment.
This variable possible not initialized yet when access make a instance use constructor.
I'm already met this kind of issue when I run unit test use this library.
I think one more variable init checking required, at least If constructor want to check unsupported or not.

* Initialize static variable from static method instead of static block

* Initialize static variable from static method instead of static block

#1671 not worked for me.
Run CI with snapshot version but I saw same error message below.
```
[ERROR] display as string  Time elapsed: 0.016 s  <<< ERROR!
java.lang.UnsupportedOperationException: Operating system not supported: JNA Platform type 2
Caused by: java.lang.UnsupportedOperationException: Operating system not supported: JNA Platform type 2
```
JNA could recognize current os in static block when failed on a constructor of SystemInfo class.
So I tried check unknown environment by JNA method instead of OSHI. At the result my CI passed successfully.
I'm not sure what is exact reason. But I just think that JNA and OSHI depends on many static initialization and it's make not easy to assume proper working.

In my opinion, static blocks and variables should be minimized.
But if OSHI have important reason to check unsupported environment in constructor this MR need to approved.

There is remained riddle why OSHI's static variable and JNA static method result don't have consistency.
Anyway, This change working for me.
Please consider enough.

* Update SystemInfo.java

* Add comment to explain why constructor is empty

Co-authored-by: sam <sam@jennifersoft.com>
Co-authored-by: Daniel Widdis <widdis@gmail.com>
dbwiddis pushed a commit that referenced this pull request Jul 18, 2021
* Initialization timing issue for static variable

currentPlatform variable initialized in static block of this class.
But it's also accessed via constructor for validate unsupported environment.
This variable possible not initialized yet when access make a instance use constructor.
I'm already met this kind of issue when I run unit test use this library.
I think one more variable init checking required, at least If constructor want to check unsupported or not.

* Initialize static variable from static method instead of static block

Co-authored-by: sam <sam@jennifersoft.com>
dbwiddis added a commit that referenced this pull request Jul 18, 2021
…1680)

* Initialization timing issue for static variable

currentPlatform variable initialized in static block of this class.
But it's also accessed via constructor for validate unsupported environment.
This variable possible not initialized yet when access make a instance use constructor.
I'm already met this kind of issue when I run unit test use this library.
I think one more variable init checking required, at least If constructor want to check unsupported or not.

* Initialize static variable from static method instead of static block

* Initialize static variable from static method instead of static block

#1671 not worked for me.
Run CI with snapshot version but I saw same error message below.
```
[ERROR] display as string  Time elapsed: 0.016 s  <<< ERROR!
java.lang.UnsupportedOperationException: Operating system not supported: JNA Platform type 2
Caused by: java.lang.UnsupportedOperationException: Operating system not supported: JNA Platform type 2
```
JNA could recognize current os in static block when failed on a constructor of SystemInfo class.
So I tried check unknown environment by JNA method instead of OSHI. At the result my CI passed successfully.
I'm not sure what is exact reason. But I just think that JNA and OSHI depends on many static initialization and it's make not easy to assume proper working.

In my opinion, static blocks and variables should be minimized.
But if OSHI have important reason to check unsupported environment in constructor this MR need to approved.

There is remained riddle why OSHI's static variable and JNA static method result don't have consistency.
Anyway, This change working for me.
Please consider enough.

* Update SystemInfo.java

* Add comment to explain why constructor is empty

Co-authored-by: sam <sam@jennifersoft.com>
Co-authored-by: Daniel Widdis <widdis@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants