-
Notifications
You must be signed in to change notification settings - Fork 84
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
NuProcessBuilder.start
does not return null
, if process cannot be started
#114
Comments
Just found that #66 and #67 would probably be fixed by changing |
I'm not sure if this should be fixed, because it might break programs, that depend on |
So, what is the correct way then to check if the process has been correctly started? Calling |
Also see #67, BTW. |
Per @brettwooldridge on #66, the way to tell it wasn't started is to have your |
The correct way depends on the definition or view of "correct". Architecturally the best way would be to fix this as I wrote above. By now, I believe the best way to fix this problem is to change the documentation to reflect the current behavior and add another property to ProcessFactory that indicates that process start failed. That property would be |
"Correct" means the way the library author has indicated it should be done, regardless of whether we think that's the best way or not. To that definition, there is only one correct way. Returning Adding a property on |
I agree. I disagree about what the "correct" way is, though. The author has stated his intention in the documentation, which clearly states that To get technical again: if |
@thraidh Looking at the code, it appears that only the Windows implementation does not return null from start(). Is that the issue you are reporting? If that is the case, that is due to a limitation of Windows, and a rather infamous limitation at that. From the Windows createProcessA documentation:
This means that createProcessA will return "successful launch!" only to have the process fail some tens or hundreds of milliseconds later due to a DLL failure. And there is no API to tell you "initialization has completed". So, you would have to poll "GetExitCodeProcess" in start(), for some arbitrary amount of time. So, how long do you wait? 100ms? Initialization could still fail after that. One second? Still no guarantee that DLL initialization is complete. Whatever you can conceive of that would allow a return of This means there is no cross-platform way to either return The decision was made long ago to return the NuProcess instance so that, for example, the user can poll the isRunning() method or call waitFor() with some timeout. But as noted in #66, the best way to detect failure is the onExit() method in the callback. The JavaDoc is incorrect, and I will fix it. |
The problem is, that Therefore making it work like the documentation says, would be very easy. The only problem is, that it could break existing programs, that are ignoring the documentation and assume, it would always work. Interestingly this includes the unit tests. In my opinion the way to go forward is to fix the documentation and add another feature to NuProcess, where you can query for startup failure, so the information that is available for Linux and OSX is not discarded and needs to be reconstructed with some workaround. That would be just in
and of course the addition of a single boolean property to |
There's not really any reason for the factory to get involved in setting such a That might fix your "pet hate" and your use case, but there are still some pretty significant problems with it:
It's not my library, but I'd prefer a documentation change to remove the incorrect assertion that Edit: Actually, I suspect for the sort of delayed initialization error described in the |
That's true, but there are other failure modes where startup detection does work--and that NuProcess's Edit: Given |
This got me curious, because I doubted process start on Linux handles this case either, so I wrote a simple test and ran it on Linux to see what would happen. First, I created an incredibly basic shared library:
Running that without
If I run that same process using NuProcess, as I expected:
So the sort of case the To me, this implies there's no appreciable difference between NuProcess's ability to detect startup failure on Windows vs. any other OS. That further implies that there should be a mechanism for relaying failure to start for the cases that can be detected that works on all platforms NuProcess supports and, ideally, that would work for both |
@bturner @thraidh First, we're all agreed that the JavaDoc does not reflect the current behavior and should be fixed. My proposal is this:
This provides a handling route for direct interrogation or callback-based notification. In both cases, the setting of state or callback notification will only occur in the case of synchronous failure, i.e. executable not found. The rub is that neither can be done in the case of post-launch initialization failure, such as missing shared library. From the standpoint of my company's use-case, this notification is not particularly informative. A non-zero exit code is the only thing we care about, or from understanding our own expectation of process interaction, the premature exit of a process whether non-zero exit or not. EDIT: If desired, the |
Adding a My personal preference is for a behavior change: Throw an exception if the process doesn't start. The NuProcess code already does this, so it's mostly "just" a matter of letting that exception through. Of course, similar to raising the minimum JDK version, that behavior change would be semantics-breaking. My proposal is to make it opt-in:
This way, for use cases that don't need to differentiate failure to start there's no change in behavior, and for use cases that do they can opt into receiving an exception. (Perhaps an One reason I like this proposal is because it works just as well for That's my 2 cents, for whatever it's worth. |
@bturner Yes, I understand regarding binary compatibility. A change in the major digit of the semantic version is required. As Java 7 is past end of life, I have no problem with that. Thanks for the feedback, I want some time to consider all of this. My overall reservation is this. While it seems like solution, it is only partially so, because it doesn't cover (and can't cover) all "launch failures". Most users, including myself, likely consider failure due to a missing shared library as a semantically identical lunch failure to a missing executable. I posit that executables that depend upon shared libraries, whether OS or internal, far outnumber those that are fully statically linked, in the wild. The end result is that the user ultimately has to handle both failure modes, as handling only failures such as missing executables by catching an exception would basically be a bug in the user's code -- missing an entire class of launch failures. Given that fact, is it not the case that the existing recommended pattern covers both cases? Handling failures in |
I'm not sure that I agree that a binary that exists but is corrupted in a way that prevents it from starting (e.g. missing some dynamic libraries) is semantically the same as a binary which does not exist at all. If it were a web page, you'd expect a 404 for the missing URL (no such binary), but a different error (5XX probably) for it being in an invalid state. With the current NuProcess code it's not really possible to differentiate between the two. Personally, to be clear, I don't really have a significant investment here. My handlers are already coded to check for Put differently, I think there's value in NuProcess having a mode that's consistent with |
To chime in from the perspective of another (future) user of this library: I'd agree here. As a user I don't really care why the process didn't start, i.e. whether it's due to a synchronous error (executable not found) or an asynchronous error (DLL / shared object not found after launch). While the difference might be valuable information to the end-user (i.e. the user of my app using NuProcess) to debug the issue, just forwarding the underlying exception message probably gives enough clue to the end-user. IMO there's no need for NuProcess to explicitly expose the difference between the synchronous and asynchronous error cases when a process starts.
Agreed, same here.
It's semantically equivalent on a higher level. Think of it like running (JUnit) tests: There are test failures and test errors. Test failures mean that the test was successfully run, but the actual results did not match the expected results. Test errors mean that there was a more fundamental problem in running the test framework itself, like a misconfiguration or a bug in JUnit. These are the two semantically different error / failure cases I'd like to distinguish on my end. So again from a fresh / new users perspective, I was puzzled to see that there are cases where Bottom line, in my personal opinion there should be a contract saying that |
@sschuberth I think you've missed a key point of the discussion. That being that a failure caused by, for example, a missing shared library, cannot be thrown as an exception from NuProcess started life as an entirely asynchronous framework. The only "notification" of any activity was through the callback API. Some users lobbied to fully support single-threaded execution, and thus this functionality was basically "bolted on" after the fact, and an attempt made to disrupt the code as little as possible in so doing. The invocation of the Therefore, in the asynchronous handler model, it makes perfect sense for there to exists scenarios in which the Of course, one could introduce a separate callback, In point of fact, a "delayed launch failure" (e.g. missing shared library) is indistinguishable from any other process exit, including normal process exit. Only the user can interpret the The only debate resolves around whether certain launch failures, i.e. the synchronous variety (no executable), should be immediately reported by throwing an exception from My point in all of my debate on this topic is that because the user still has to handle delayed launch failures via a callback, the prospect of handling failures in two places in the code rather than a single place, is less desirable. Again, at least in the opinion of this engineer. But again I am amenable to implementing a non-breaking change in behavior. |
In my use case users can configure a script that is called as a callback for certain actions. Today all these are caught with the "on(Pre)Start-not-called" method, but that is not very discoverable. Therefore I'd like to have a method like With that method an optional exception can be as easy as:
but I see the allure of the library providing that, and you just need
Anyway, the exception should be in addition to the Regarding the handler, it seems to be a little bit more complicated. If you do not want to break compatibility,
The second option would also not require Java 8, because you do not need a default method in the interface. |
According to the documentation
NuProcessBuilder.start
should returnnull
, if there is an immediately detectable launch failure.Unfortunately (at least on Linux) it does not.
The problem is
LinProcessFactory.createProcess
, whereshould be
as
process.start
correctly returnsnull
or the actual process instance. The factory on the other hand always returns the process instance, even if it could not be started.The text was updated successfully, but these errors were encountered: