-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Thread dump for java debugger #5320
Conversation
Build # 2773 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2773/ to view the results. |
Build # 2809 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2809/ to view the results. |
Build # 2822 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2822/ to view the results. |
Build # 2831 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2831/ to view the results. |
Build # 2838 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2838/ to view the results. |
Build # 2844 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2844/ to view the results. |
Build # 2848 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2848/ to view the results. |
Signed-off-by: Oleksii Orel <oorel@redhat.com>
Signed-off-by: Oleksii Orel <oorel@redhat.com>
|
||
/** @author Anatolii Bazko */ | ||
@Singleton | ||
public class DebuggerResourceHandlerFactory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid term Factory
here, because factory is creational pattern. This class just returns one of registered or default object.
@@ -38,12 +41,22 @@ | |||
void onExpandVariablesTree(); | |||
|
|||
/** | |||
* Performs any actions appropriate in response to the user having selected variable in | |||
* Performs any actions appropriate in response to the user having selected variable in* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably asterisk is added occasionally.
|
||
for (int i = 0; i < threadDumps.size(); i++) { | ||
ThreadDump td = threadDumps.get(i); | ||
String item = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider usage of StringBuilder
please.
@@ -21,9 +21,10 @@ | |||
import org.eclipse.che.ide.util.dom.Elements; | |||
|
|||
/** | |||
* The rendered for debug variable node. | |||
* Renders variable item the panel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commentary isn't clear to me.
} | ||
|
||
@Override | ||
public String evaluate(String expression, long threadId, int frameIndex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method above use lock to prevent simultaneous execution of this one. What if someone call this public method directly?
} | ||
|
||
@Override | ||
public SimpleValue getValue(VariablePath variablePath, long threadId, int frameIndex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as with evaluate
method. Someone could use it directly bypassing lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locks are legacy. will be eliminated soon. The right way not to use them.
public List<Variable> getVariables() { | ||
if (variables.get() == null) { | ||
synchronized (variables) { | ||
if (variables.get() == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you check the same 2 lines above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid setting variables simultaneously in different threads.
* | ||
* @author Anatolii Bazko | ||
*/ | ||
public class EvaluateExpressionTest1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below. Why do you use 1
in the name of tests? If we have a few test classes, maybe we should give them better names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there will be a lot of tests using different compiled test classes.
Right now I don't see the appropriate names for them.
javac -g org/eclipse/EvaluateExpressionTest1.java | ||
javac -g com/HelloWorld.java | ||
|
||
java -Xdebug -Xrunjdwp:transport=dt_socket,address=8001,server=y,suspend=y org.eclipse.ThreadDumpTest1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move common options into a variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant create a variable with -Xdebug -Xrunjdwp:transport=dt_socket,address=8001,server=y,suspend=y
and then use it, for example:
java $args org.eclipse.ThreadDumpTest1
Easier to read. But think of better name, of course.
@@ -29,4 +31,11 @@ | |||
|
|||
/** Returns project path, for resource which we are debugging now. */ | |||
String getResourceProjectPath(); | |||
|
|||
/** Returns the method is being executed. */ | |||
@Nullable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment when this method returns null please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to predict.
@@ -19,4 +20,8 @@ | |||
|
|||
/** The list of local variables. */ | |||
List<? extends Variable> getVariables(); | |||
|
|||
/** Returns location of the frame. */ | |||
@Nullable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment when this method returns null please
Build # 3510 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/3510/ to view the results. |
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/3627/ |
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/3638/ |
Build # 3650 - FAILED Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/3650/ to view the results. |
ci-build |
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/3655/ |
CHE-3708: Thread dump for java debugger
What does this PR do?
Introduces thread feature for debugger (client side).
Adds implementation for java debugger.
What issues does this PR fix or reference?
#3708
#3712
Changelog
Adds thread dump feature for java debugger
Release Notes
Adds thread dump feature for java debugger