[JDK17] Update working with PIDs to be compatible with JDK9+ #123
Conversation
|
Note that this is still WIP. More check and tests should come but I've decided to at least share this here so you can freely comment and raise your concerns. |
jstourac
force-pushed
the
jdk17
branch
2 times, most recently
from
a8c625c to
ac898e8
Compare
jstourac
force-pushed
the
jdk17
branch
2 times, most recently
from
b76e296 to
cd2ebf6
Compare
| try { | ||
| Field f = process.getClass().getDeclaredField("pid"); | ||
| f.setAccessible(true); | ||
| processId = f.getInt(process); |
There was a problem hiding this comment.
Is this OK? I can see the implementation of windows-specific function was updated, but not this one.
There was a problem hiding this comment.
Hm, good point - I updated this to getLong too, since the field is Long in new JDKs implementations. Still this should not break with the JDK8 hopefully :)
| pidAsStr = pidAsStr.trim() | ||
| pidAsStr = pidAsStr.replaceAll('"', '') | ||
| pid = Integer.valueOf(pidAsStr) | ||
| pid = Long.parseLong(pidAsStr) |
There was a problem hiding this comment.
Any reason why you are using Long.parseLong() instead of Long.valueOf() ?
There was a problem hiding this comment.
hm, truth, I suppose that we should stick with what it was here before. Also, since we are supposed to return Long (function definition), we actually should rather use valueOf. Updated.
There was a problem hiding this comment.
@AdamKrajcik, I've updated also other occurrences of the parseLong back to valueOf where applicable.
jstourac
force-pushed
the
jdk17
branch
2 times, most recently
from
cdca5b7 to
c6cbbc3
Compare
|
@AdamKrajcik thank you for your review. I've fixed both places and also rebased against latest |
|
Ready for another round. |
core/src/main/groovy/noe/common/utils/processid/ProcessUtils.groovy
Outdated
Show resolved
Hide resolved
core/src/main/groovy/noe/common/utils/processid/ProcessUtils.groovy
Outdated
Show resolved
Hide resolved
AdamKrajcik
left a comment
AdamKrajcik
left a comment
There was a problem hiding this comment.
Everything looks fine by me.
| * TODO: properly implement it for Unix/Linux systems as pkill -P kills only processes with given parent PID, it doesn't go recursively killing from bottom to top | ||
| */ | ||
| static int killTree(Integer pid, winappname) { | ||
| static Long killTree(Long pid, winappname) { |
There was a problem hiding this comment.
@jstourac please can you verify the return type as looking at the code it could have remained an int, if I'm not mistaken.
There was a problem hiding this comment.
@PaulLodge, you're right, thanks for the catch - I've just pushed the fix. I'll also start internal test jobs again, just to be sure
Even though this change doesn't makes this project able to compile with JDK9+ it at least adds a possibility to utilize its classes and methods in some other projects with regards the work with PIDs running on JDK9+. We are using a "hack" to call the method for PID retrieval by reflection because it is only present in Java 9 and newer. That allows us to still compile this code on older JDKs. As a consequence the pid number changed from integer to long so we had to change it everywhere. Dependent projects should update their usages appropriately to long too, although since long -> integer is just about the precision loss issue and I am not aware of any platform (from those we use) that uses long instead of integer for their PIDs, it shouldn't be a big issue.
3 participants
Even though this change doesn't makes this project able to compile with
JDK9+ it at least adds a possibility to utilize its classes and methods
in some other projects with regards the work with PIDs running on JDK9+.
We are using a "hack" to call the method for PID retrieval by reflection
because it is only present in Java 9 and newer. That allows us to still
compile this code on older JDKs.
As a consequence the pid number changed from integer to long so we had
to change it everywhere. Dependent projects should update their usages
appropriately to long too, although since long -> integer is just about
the precision loss issue and I am not aware of any platform (from those
we use) that uses long instead of integer for their PIDs, it shouldn't
be a big issue.
This is slightly related to what is being prepared here #120.