Community
Participate
Working Groups
Created attachment 197756 [details] Implementation Allow to interrupt a thread from the OSGi console by sending it a RuntimeException via Thread#stop(Throwable). E.g. in bug 339021, I was caught in an endless loop in the main thread, and it would have been quite helpful, if I could just have used the OSGi console to throw an exception, thus reviving the display loop. [1] The attached patch implements this as an extension to the "threads" command: > threads stop main I'm not sure if exception.setStackTrace(t.getStackTrace()); is allowed in the org.eclipse.osgi bundle, since Thread#getStackTrace() is @since 1.5. If not, you can just remove that line and the lines before and after that deal with the 'cause' variable. [1] Actually, I managed to escape from there, but it was a pain. I used http://www.adaptj.com/main/stacktrace , opened a Remote BeanShell Console, and then ran the command > thread("main").stop(new IllegalStateException("injected"));
One for the Juno plan? I just had someone in my office whose Eclipse was hung in the UI thread. This little patch could have helped recovering much faster.
I'm not sure I like the usage of calling Thread.stop() this seems rather dangerous. But one can argue that the "exit" command that calls System.exit() is as dangerous as it gets ;-) CC Lazar since we are attempting to extract the console and all these built-in commands out to a separate bundle which Lazar has been working on.
Created attachment 200214 [details] updated patch (In reply to comment #0) > I'm not sure if > exception.setStackTrace(t.getStackTrace()); > is allowed in the org.eclipse.osgi bundle, since Thread#getStackTrace() is > @since 1.5. If not, you can just remove that line and the lines before and > after that deal with the 'cause' variable. > We actually don't have getStackTrace() or stop() available in the osgi min ee 1.2. Here is a (git) patch that uses reflection.
Moving to components to consider adding this feature to the external equinox.console bundle.
This would be a nice time saver!
See also feature 392893.
We should release this to the external console commands in M4.
(In reply to comment #2) > I'm not sure I like the usage of calling Thread.stop() this seems rather > dangerous. But one can argue that the "exit" command that calls > System.exit() is as dangerous as it gets ;-) > Is using Thread.stop() no longer a concern? I assume it's already been noted that this is a deprecated method? Also, I assume we're willing to let the user accept full responsibility for any resulting arbitrary behavior as described in the Thread javadoc? Is it envisioned this would only be used as a method of last resort when troubleshooting, and there is no expectation of being able to continue running a stable environment for any length of time? Finally, I assume Thread.interrupt() was not considered instead over concern that the target thread might not respond to the interrupt?
It seems the main motivation here is to guarantee that a thread that is behaving badly will stop running. Using IllegalStateException extends RuntimeException extends Exception could be problematic if the exception is caught and ignored at some level. It's not uncommon for something high up in the stack to catch (Exception), log it, then move on. From this perspective, it would probably be better to use Thread.stop() with no arguments so that a ThreadDeath error is thrown. Although the same issue described above potentially exists here as well, it is less likely that errors are being caught and ignored. I understand that the motivation of using ISE is so that a message can be provided indicating that the OSGi console stopped the thread. Unfortunately, ThreadDeath does not have a constructor that takes a String, so it can't be used with the stop(Throwable) method. So a third alternative would be to define our own OSGiConsoleError extending Error, then use that as the argument to Thread.stop.
(In reply to comment #8) > Is using Thread.stop() no longer a concern? It is a concern, but this facility is indeed only intended as a last resort before killing the process or invoking "exit". Having a more or less controlled way to throw a RuntimeException is preferable to killing the whole VM. BTW: The stop(Throwable obj) is not as bad as stop(). The only unusual thing about the former is that the exception comes out of nowhere, rather than from a place that is allowed to throw an exception according to the Java Language Specification. In the patch, I propose to throw an IllegalStateException, which is a RuntimeException that can really happen almost everywhere in real life code. So most of the concerns in the deprecation message don't apply here. > Finally, I assume Thread.interrupt() was not considered instead over concern > that the target thread might not respond to the interrupt? Yes. Thread.interrupt() won't help in most cases to stop an endless loop. It could be useful in other situations, but I would only add support for this if somebody has a real use case. (In reply to comment #9) > From this perspective, it would probably be better to use Thread.stop() with > no arguments so that a ThreadDeath error is thrown. No. ThreadDeath is explicitly *not* the goal of my request. I don't want to kill the whole thread. I just want to get out of an endless loop (e.g. in the UI thread). If you want to support throwing arbitrary exceptions (including e.g. ThreadDeath), then the syntax could be extended like this: threads [stop <thread name> [<exception name>]] BTW: Now that equinox has moved to 1.5, you should not need the reflective hoops in the "updated patch" any more.
(In reply to comment #10) > I just want to get out of an endless loop (e.g. in the UI thread). I.e. I want the exception to be caught and handled as a normal runtime exception.
Created attachment 223083 [details] Proposed final patch This patch supports stopping threads via the OSGi console. The existing threads command was updated to support additional syntax. The full syntax is now (t | threads) [stop <thread> [<throwable> [<msg>]]] where <thread> is the thread name, <throwable> is the fully qualified class name of something extending java.lang.Throwable and visible to the console bundle's class loader, and <msg> is the throwable's message. If <throwable> is unspecified, Thread.stop() is invoked, resulting in a ThreadDeath error being thrown in the target thread. If <throwable> is specified but <msg> is not, a new instance of the class will be created using the default constructor. If both <throwable> and <msg> are specified, a new instance of the class will be created using the constructor that accepts a String as the sole argument. The <throwable> will be initialized with the target thread's stack trace and a cause of java.lang.RuntimeException containing a translatable message indicating the thread was stopped by the OSGi console. The cause will contain the stack trace of the current thread. As an example, the command to use for the use case mentioned in this bug (see comment 0) would be threads stop main java.lang.IllegalStateException inject I'll release the contents once the repository opens up again after M3.
I don't have a use case for sending ThreadDeath, and I doubt that this is what most users need. The default should really be a RuntimeException like in the original patch. If someone really needs java.lang.ThreadDeath, then they can easily specify that explicitly.
(In reply to comment #13) > I don't have a use case for sending ThreadDeath, and I doubt that this is > what most users need. The default should really be a RuntimeException like > in the original patch. > > If someone really needs java.lang.ThreadDeath, then they can easily specify > that explicitly. Why should the default be RuntimeException (specifically, it was IllegalStateException in the patch)? We're issuing a stop thread command. It seems more intuitive to have it follow the Java API. As a user, if I don't specify an exception, I would expect Thread.stop() to be called. I would be surprised by a RuntimeException. If not otherwise indicated by the user, the command makes no assumptions and follows the default behavior of the Java API. If a user does not want the default behavior, they specify the desired throwable.
(In reply to comment #14) > (In reply to comment #13) > > I don't have a use case for sending ThreadDeath, and I doubt that this is > > what most users need. The default should really be a RuntimeException like > > in the original patch. > > > > If someone really needs java.lang.ThreadDeath, then they can easily specify > > that explicitly. > > Why should the default be RuntimeException (specifically, it was > IllegalStateException in the patch)? We're issuing a stop thread command. Because it is less destructive/dangerous.
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > I don't have a use case for sending ThreadDeath, and I doubt that this is > > > what most users need. The default should really be a RuntimeException like > > > in the original patch. > > > > > > If someone really needs java.lang.ThreadDeath, then they can easily specify > > > that explicitly. > > > > Why should the default be RuntimeException (specifically, it was > > IllegalStateException in the patch)? We're issuing a stop thread command. > > Because it is less destructive/dangerous. Don't we already assume that anyone with access to the console knows what they're doing? There are plenty of destructive and dangerous things already there. Why hire an adult then treat them like a child? I frankly thought your argument was going to be from convenience. That is, you speculate that your use case will be the most common one, and it is convenient not to have to type out the exception name. I thought this was reasonable as well, although I personally still like the approach in the current patch better for the reasons given in comment 14. In a nutshell, console users will be familiar with the Java Thread API. The current patch mimics the API and is intuitive. The alternative approach is not intuitive and requires explanation. Anyone else want to express a preference?
The people who requested the change expressed their preference, so I don't see why you think this needs to be implemented differently. Please come up with *other* people that actually request what you plan to implement.
(In reply to comment #14) > Why should the default be RuntimeException (specifically, it was > IllegalStateException in the patch)? I said "a RuntimeException". IllegalStateException is a RuntimeException, but it can also be a different RuntimeException. > We're issuing a stop thread command. It > seems more intuitive to have it follow the Java API. That's a theoretical observation, but it's not relevant in practice. No sane Java programmer uses Thread#stop() or Thread#stop(Throwable) if there's a different solution. Furthermore, no harm is done if the command throws an ISE instead of a ThreadDeath. But throwing a ThreadDeath is much more harmful than throwing an ISE, so that shouldn't be the default. An interactive console is different from an API; you should not copy APIs 1:1 to a console command. If the command name is the problem, then you can also use a different name, e.g. disrupt, throwISE, ... .
(In reply to comment #17) > The people who requested the change expressed their preference, so I don't > see why you think this needs to be implemented differently. Please come up > with *other* people that actually request what you plan to implement. John did express his reasoning for his implementation choices in comment 14 and I think his reasons make sense if we want to follow the Thread API closely: osgi> t main stop Should then simply call Thread.stop(). But nothing says we must follow the Java API directly. The help for the command can simply indicate that Thread.stop(Throwable) is always used and IllegalArgumentException is used by default. I don't have a huge opinion in this matter because I don't really think it matters much either way. I propose the command use the following arguments: (t | threads) [stop <thread name> [<msg> [<throwable>]]] Where the default throwable is IllegalArgumentException and the default message is "Thread '<thread name>' stopped via OSGi console". In the case where the throwable class does not have a (String) constructor then the message is only used for the RuntimeException cause.
(In reply to comment #19) > Where the default throwable is IllegalArgumentException I meant IllegalStateException as the default.
(In reply to comment #19) > John did express his reasoning for his implementation choices in comment 14 > and I think his reasons make sense if we want to follow the Thread API > closely: Yes, I got that, but he did not request the feature. What I was asking, is whether there are others that want a different behavior than what was requested by Markus and me. And if so, for what *practical* reasons i.e. for what use cases.
(In reply to comment #21) > Yes, I got that, but he did not request the feature. Just because you request the feature does not mean you have the absolute authority on the final solution ;-) At any rate, I think my suggested solution is fine and I would prefer we implement that and get this done with.
Why would anybody want to type in a message when killing a thread on the console? I don't see a practical use case for this. The <msg> should go away. We can just set the default message if possible. To restate the initial request: Add a simple facility for getting out of an endless loop.
(In reply to comment #23) > The <msg> should go away. We can just set the default message if possible. Fine with me.
Good, so we're back to comment 0, plus comment 10 if you want: > If you want to support throwing arbitrary exceptions (including e.g. > ThreadDeath), then the syntax could be extended like this: > threads [stop <thread name> [<exception name>]]
Created attachment 223204 [details] Proposed final patch Updated patch based on the consensus. The syntax is now (t | threads) [stop <thread> [<throwable>]] where <thread> is the thread name, and <throwable> is the fully qualified class name of something extending java.lang.Throwable and visible to the console bundle's class loader. If <throwable> is unspecified, the default is java.lang.IllegalStateException. The <throwable> will be created with a translatable message indicating the thread was stopped by the OSGi Console, if a constructor accepting a String as the sole parameter exists. Otherwise, the default constructor is used. The <throwable> is initialized with the target thread's stack trace and a cause of java.lang.RuntimeException which will also contain the translatable message. The cause will contain the stack trace of the current thread.
Pushed to master with http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=ec21eb4e37d764934e84fac5ee09179f3d66b12b.
Thanks, this works fine now. But the help is hard to read: osgi> help threads threads - perform the specified action on the named thread using the provided throwable; supported actions are {stop} scope: equinox parameters: String the action to perform String the thread on which to perform the action Class the class of the throwable to be used in conjunction with the action (default = java.lang.IllegalStateException) threads - display threads and thread groups scope: equinox threads - perform the specified action on the named thread; supported actions are {stop} scope: equinox parameters: String the action to perform String the thread on which to perform the action => the 3-args command should be the last in the list => the description of the stop action is unnecessarily complicated. As long as there's just one action, the description should just spell it out: threads - display threads and thread groups scope: equinox threads - stop the named thread with an IllegalStateException scope: equinox parameters: String stop String the thread on which to perform the action threads - stop the named thread with the provided throwable scope: equinox parameters: String stop String the thread on which to perform the action Class the class of the throwable to throw (default = java.lang.IllegalStateException)
(In reply to comment #28) > Thanks, this works fine now. But the help is hard to read: > > osgi> help threads > > threads - perform the specified action on the named thread using the > provided throwable; supported actions are {stop} > scope: equinox > parameters: > String the action to perform > String the thread on which to perform the action > Class the class of the throwable to be used in conjunction with the > action (default = java.lang.IllegalStateException) > > threads - display threads and thread groups > scope: equinox > > threads - perform the specified action on the named thread; supported > actions are {stop} > scope: equinox > parameters: > String the action to perform > String the thread on which to perform the action > > => the 3-args command should be the last in the list That's strange because the commands show up in the preferred order in my console. Not sure why yours is different or what controls the ordering. > => the description of the stop action is unnecessarily complicated. As long > as there's just one action, the description should just spell it out: Okay. Alternatively, we could remove the need to specify an action altogether until there's need for another. > > threads - display threads and thread groups > scope: equinox > > threads - stop the named thread with an IllegalStateException > scope: equinox > parameters: > String stop > String the thread on which to perform the action > > threads - stop the named thread with the provided throwable > scope: equinox > parameters: > String stop > String the thread on which to perform the action > Class the class of the throwable to throw (default = > java.lang.IllegalStateException)
> That's strange because the commands show up in the preferred order in my > console. Not sure why yours is different or what controls the ordering. Probably a HashMap or HashSet somewhere that should be turned into a LinkedHash*. > Okay. Alternatively, we could remove the need to specify an action > altogether until there's need for another. "stop" must be part of the command. Throwing an exception on "threads main" would be very confusing. But the action could be turned into a flag, so that a command would e.g. read "threads -stop main". However, the plain "stop" is just as good.
(In reply to comment #30) > > That's strange because the commands show up in the preferred order in my > > console. Not sure why yours is different or what controls the ordering. > > Probably a HashMap or HashSet somewhere that should be turned into a > LinkedHash*. Control for printing help is passed to org.apache.felix.service.command.CommandSession.execute, so it seems this would need to be addressed there. Perhaps Lazar would have some insight? > > > Okay. Alternatively, we could remove the need to specify an action > > altogether until there's need for another. > > "stop" must be part of the command. Throwing an exception on "threads main" > would be very confusing. But the action could be turned into a flag, so that > a command would e.g. read "threads -stop main". However, the plain "stop" is > just as good. I committed updates to the help wording with http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=82e7c169846f5fd65aeb5ef9758ef9cbb032184f.
Thanks. Could you also fix the new unused import that shows up in the build? http://download.eclipse.org/eclipse/downloads/drops4/I20121106-0800/compilelogs/plugins/org.eclipse.equinox.console_1.0.100.v20121105-213654/@dot.html#OTHER_WARNINGS Hint: Enable Organize Imports on Preferences > Java > Editor > Save Actions.
(In reply to comment #32) > Thanks. Could you also fix the new unused import that shows up in the build? > > http://download.eclipse.org/eclipse/downloads/drops4/I20121106-0800/ > compilelogs/plugins/org.eclipse.equinox.console_1.0.100.v20121105-213654/ > @dot.html#OTHER_WARNINGS > > Hint: Enable Organize Imports on Preferences > Java > Editor > Save Actions. Fixed in http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=c57232c0d071ba6cc0d7636782ada02c6fcc8b02.
(In reply to comment #31) > (In reply to comment #30) > Control for printing help is passed to > org.apache.felix.service.command.CommandSession.execute, so it seems this > would need to be addressed there. Perhaps Lazar would have some insight? Gogo uses reflection on command services to determine command methods. After that it stores them in ArrayLists. Since Class.getMethods() returns the methods in the same order each time, they should always be displayed in the same order. It is strange that the opposite is observed. I will further investigate this.
(In reply to comment #34) > Since Class.getMethods() returns the > methods in the same order each time, they should always be displayed in the > same order. It is strange that the opposite is observed. I will further > investigate this. No need to waste more investigation time. The problem is that Class#getMethods() and #getDeclaredMethods() used to work fine before Java 7, although this was never specified. Then, Oracle decided to actively screw all clients by returning methods in an unpredictable order. The right solution for the java.lang.Class APIs is to strengthen the method contracts and return the methods in declaration order. Instead, they decided that this is "not an issue": http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7023180 For Gogo, I think the best workaround is to sort the methods by name, parameter count, and parameter type (in that order).
