Move supported operating system check out of SystemInfo constructor #1680
Conversation
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.
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.
|
Thank you for testing and for the new proposed fix. I don't disagree with the code, but until I understand exactly what the root cause of the error is, I am hesitant to accept it. It could be some sort of race condition that your proposal appears to fix but doesn't. I would really like to try to reproduce this myself. Can you tell me what CI system you are using? If this is not pure Java, what language is involved (is this Kotlin?). Based on the initial report I suspect having the code in the constructor may be the root cause, and your suggested fix may not be permanent. There is probably a bug in some external program somewhere and it is important to try to reproduce it and fix it, rather than just trying workarounds. |
Generally true, but:
The existing code follows the order of initialization of static variables/execution of static blocks in Java. If non-Java code is not observing this convention, there is a bug in that non-Java code that should be fixed, and fixing that code requires finding a reproducible example. If it is not a bug but a "feature" of that other code (e.g., Kotlin treats default constructors differently) then the reason that language violates Java conventions should be completely understood so it can be properly defended against. |
|
Yes, That's important to find root cause. |
|
I was test one more thing.
public class StorageOnSystemJava {
public StorageOnSystemJava(SystemInfo info) {
}
}
class StorageOnSystemJavaTest {
@Test
void run() {
new StorageOnSystemJava(new SystemInfo());
}
}
As I know, kotlin is Interoperable with java perfectly. |
I agree with efficiency is very important. |
|
Thank you for the detailed description of the problem and the simplified test case. I will try to reproduce it myself using that code. |
|
Hi @KyongSik-Yoon I'm still trying to get to the root cause here. I have some questions about your testing setup. You indicated "This error also happened when run maven install or test task without CI." and the debug output above shows only a single test running. But in your earlier report you said "When I run single unit test. There is no problem." Can you clarify what you mean by these two statements?
Are you able to edit OSHI's code to add some log debug statements or use a debugger on the process and execute with that modified code? The part where it looks like it's failing is where it checks |
dbwiddis
changed the title
dbwiddis
changed the title
…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>
|
So I've come to to the conclusion that it's probably Kotlin's "constructor first" behavior causing something strange here. I reverted to the OSHI 5.2.x setup for the 5.8.0 release. I'd still like to try to definitively identify the problem, which may be a bug in how Kotlin interacts with the JVM/JLS. |
|
Sorry for my late response. |
|
|
But the error occurs when running under Kotlin testing ( |
#1671 not worked for me.
Run CI with snapshot version but I saw same error message below.
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.