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

Add new methods to get current process ID (pid) to KiwiEnvironment #640

Closed
sleberknight opened this issue Dec 7, 2021 · 2 comments · Fixed by #641 or #644
Closed

Add new methods to get current process ID (pid) to KiwiEnvironment #640

sleberknight opened this issue Dec 7, 2021 · 2 comments · Fixed by #641 or #644
Assignees
Labels
new feature A new feature such as a new class, method, package, group of classes, etc.
Milestone

Comments

@sleberknight
Copy link
Member

sleberknight commented Dec 7, 2021

Replace the existing currentProcessId and tryGetCurrentProcessId methods with the following:

  • long currentPid()
  • Optional<Long> tryGetCurrentPid()

These should use ProcessHandle.current().pid() internally.

The old methods will be deprecated by #639

@sleberknight sleberknight added the new feature A new feature such as a new class, method, package, group of classes, etc. label Dec 7, 2021
@sleberknight sleberknight added this to the 1.1.1 milestone Dec 7, 2021
@sleberknight sleberknight self-assigned this Dec 7, 2021
sleberknight added a commit that referenced this issue Dec 7, 2021
* Add currentPid() and tryGetCurrentPid() to KiwiEnvironment
* Add default implementations of these methods in DefaultEnvironment

Closes #640
terezivy pushed a commit that referenced this issue Dec 7, 2021
* Add currentPid() and tryGetCurrentPid() to KiwiEnvironment
* Add default implementations of these methods in DefaultEnvironment

Closes #640
@sleberknight
Copy link
Member Author

Re-opening this to discuss whether the tryGetCurrentPid() should actually return OptionalLong instead of Optional<Long>, given that the pid is represented by Process and ProcessHandle as a long.

@sleberknight sleberknight reopened this Dec 8, 2021
@sleberknight
Copy link
Member Author

One really annoying thing is that the OptionalXxx (e.g. OptionalLong) classes in the JDK do not have a map method. In contrast, with Optional<Long>, you can map it, e.g.:

// assume pid is a Long
var desc = Optional.of(pid).map(String::valueOf).orElse("[unknown]");

But to do something similar if you have an OptionalLong, you must first convert to a LongStream, call findFirst, and then you can use the mapToObj method:

// assume optLongPid is an OptionalLong
var desc = optLongPid.stream().mapToObj(String::valueOf).findFirst().orElse("[unknown]")

I don't really understand why the JDK designers chose not to add map methods to the primitive OptionalXxx wrappers like OptionalInt and OptionalLong, but they didn't, so we have to resort to more verbose code.

The only use case I've found where this matters at all is the above one, in which we want to log the pid or a String indicating it is unknown. And the only reason that is necessary is that the JDK's pid() methods in Process and ProcessHandle may throw UnsupportedOperationException "if the implementation does not support this operation".

Overall, I think it makes the most sense to change it to OptionalLong, despite the above use case, since OptionalLong seems to be a better representation of the type, mainly since the JDK pid() methods return long.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature A new feature such as a new class, method, package, group of classes, etc.
Projects
None yet
1 participant