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

Thread dump for java debugger #5320

Merged
merged 50 commits into from
Sep 13, 2017
Merged

Thread dump for java debugger #5320

merged 50 commits into from
Sep 13, 2017

Conversation

tolusha
Copy link
Contributor

@tolusha tolusha commented Jun 9, 2017

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

screenshot from 2017-08-18 17-12-13

@tolusha tolusha added kind/enhancement A feature request - must adhere to the feature request template. status/in-progress This issue has been taken by an engineer and is under active development. labels Jun 9, 2017
@tolusha tolusha self-assigned this Jun 9, 2017
@codenvy-ci
Copy link

Build # 2773 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2773/ to view the results.

@codenvy-ci
Copy link

Build # 2809 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2809/ to view the results.

@codenvy-ci
Copy link

Build # 2822 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2822/ to view the results.

@codenvy-ci
Copy link

Build # 2831 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2831/ to view the results.

@codenvy-ci
Copy link

Build # 2838 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2838/ to view the results.

@codenvy-ci
Copy link

Build # 2844 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2844/ to view the results.

@codenvy-ci
Copy link

Build # 2848 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/2848/ to view the results.


/** @author Anatolii Bazko */
@Singleton
public class DebuggerResourceHandlerFactory {
Copy link
Contributor

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*
Copy link
Contributor

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 =
Copy link
Contributor

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.
Copy link
Contributor

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)
Copy link
Contributor

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)
Copy link
Contributor

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.

Copy link
Contributor Author

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) {
Copy link
Contributor

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?

Copy link
Contributor Author

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 {
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't understand

Copy link
Contributor

@mmorhun mmorhun Sep 4, 2017

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
Copy link
Contributor

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

Copy link
Contributor Author

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
Copy link
Contributor

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

@codenvy-ci
Copy link

Build # 3510 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/3510/ to view the results.

@codenvy-ci
Copy link

@codenvy-ci
Copy link

@codenvy-ci
Copy link

Build # 3650 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/3650/ to view the results.

@tolusha
Copy link
Contributor Author

tolusha commented Sep 13, 2017

ci-build

@tolusha
Copy link
Contributor Author

tolusha commented Sep 13, 2017

nimbus-record-video-2017-09-13-12-36-45 1

@codenvy-ci
Copy link

@tolusha tolusha merged commit 6b76cad into master Sep 13, 2017
@tolusha tolusha deleted the CHE-3708 branch September 13, 2017 10:35
skabashnyuk pushed a commit that referenced this pull request Jan 3, 2020
CHE-3708: Thread dump for java debugger
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template. status/in-progress This issue has been taken by an engineer and is under active development.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants