8000 Fix race competition when the breakpoint needs be evaluated in multi threads by testforstephen · Pull Request #325 · microsoft/java-debug · GitHub
[go: up one dir, main page]
Skip to content

Fix race competition when the breakpoint needs be evaluated in multi threads #325

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.

Sign up for GitHub

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

Merged
testforstephen merged 2 commits into from
May 7, 2020
Merged

Fix race competition when the breakpoint needs be evaluated in multi threads #325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd136a4
Fix race competition when the breakpoint needs be evaluated in multi …
testforstephen Mar 27, 2020
0178dde
Use thread-safe map
testforstephen May 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 43 additions & 5 deletions ...ft.java.debug.core/src/main/java/com/microsoft/java/debug/core/EvaluatableBreakpoint.java
View file
Open in desktop
10BC0
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,39 @@

import org.apache.commons.lang3.StringUtils;

import java.util.Map;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ConcurrentHashMap;

import com.sun.jdi.event.ThreadDeathEvent;
import com.sun.jdi.ThreadReference;
import com.sun.jdi.VirtualMachine;

import io.reactivex.disposables.Disposable;

public class EvaluatableBreakpoint extends Breakpoint implements IEvaluatableBreakpoint {
private IEventHub eventHub = null;
private Object compiledConditionalExpression = null;
private Object compiledLogpointExpression = null;
private Map<Long, Object> compiledExpressions = new ConcurrentHashMap<>();

EvaluatableBreakpoint(VirtualMachine vm, IEventHub eventHub, String className, int lineNumber) {
super(vm, eventHub, className, lineNumber, 0, null);
this(vm, eventHub, className, lineNumber, 0, null);
}

EvaluatableBreakpoint(VirtualMachine vm, IEventHub eventHub, String className, int lineNumber, int hitCount) {
super(vm, eventHub, className, lineNumber, hitCount, null);
this(vm, eventHub, className, lineNumber, hitCount, null);
}

EvaluatableBreakpoint(VirtualMachine vm, IEventHub eventHub, String className, int lineNumber, int hitCount, String condition) {
super(vm, eventHub, className, lineNumber, hitCount, condition);
EvaluatableBreakpoint(VirtualMachine vm, IEventHub eventHub, String className, int lineNumber, int hitCount,
String condition) {
this(vm, eventHub, className, lineNumber, hitCount, condition, null);
}

EvaluatableBreakpoint(VirtualMachine vm, IEventHub eventHub, String className, int lineNumber, int hitCount, String condition, String logMessage) {
EvaluatableBreakpoint(VirtualMachine vm, IEventHub eventHub, String className, int lineNumber, int hitCount,
String condition, String logMessage) {
super(vm, eventHub, className, lineNumber, hitCount, condition, logMessage);
this.eventHub = eventHub;
}

@Override
Expand Down Expand Up @@ -74,11 +87,36 @@ public Object getCompiledLogpointExpression() {
public void setCondition(String condition) {
super.setCondition(condition);
setCompiledConditionalExpression(null);
compiledExpressions.clear();
}

@Override
public void setLogMessage(String logMessage) {
super.setLogMessage(logMessage);
setCompiledLogpointExpression(null);
compiledExpressions.clear();
}

@Override
public Object getCompiledExpression(long threadId) {
return compiledExpressions.get(threadId);
}

@Override
public void setCompiledExpression(long threadId, Object compiledExpression) {
compiledExpressions.put(threadId, compiledExpression);
}

@Override
public CompletableFuture<IBreakpoint> install() {
Disposable subscription = eventHub.events()
.filter(debugEvent -> debugEvent.event instanceof ThreadDeathEvent)
.subscribe(debugEvent -> {
ThreadReference deathThread = ((ThreadDeathEvent) debugEvent.event).thread();
compiledExpressions.remove(deathThread.uniqueID());
});
super.subscriptions().add(subscription);

return super.install();
}
}
33 changes: 33 additions & 0 deletions ...t.java.debug.core/src/main/java/com/microsoft/java/debug/core/IEvaluatableBreakpoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,44 @@ public interface IEvaluatableBreakpoint {

void setLogMessage(String logMessage);

/**
* please use {@link #setCompiledExpression(long, Object)} instead.
*/
@Deprecated
void setCompiledConditionalExpression(Object compiledExpression);

/**
* please use {@link #getCompiledExpression(long)} instead.
*/
@Deprecated
Object getCompiledConditionalExpression();

/**
* please use {@link #setCompiledExpression(long, Object)} instead.
*/
@Deprecated
void setCompiledLogpointExpression(Object compiledExpression);

/**
* please use {@link #getCompiledExpression(long)} instead.
*/
@Deprecated
Object getCompiledLogpointExpression();

/**
* Sets the compiled expression for a thread.
*
* @param threadId - thread the breakpoint is hit in
* @param compiledExpression - associated compiled expression
*/
void setCompiledExpression(long threadId, Object compiledExpression);

/**
* Returns existing compiled expression for the given thread or
* <code>null</code>.
*
* @param threadId thread the breakpoint was hit in
* @return compiled expression or <code>null</code>
*/
Object getCompiledExpression(long threadId);
}
26 changes: 25 additions & 1 deletion com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/Watchpoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,20 @@
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ConcurrentHashMap;

import org.apache.commons.lang3.StringUtils;

import com.sun.jdi.Field;
import com.sun.jdi.ReferenceType;
import com.sun.jdi.ThreadReference;
import com.sun.jdi.VMDisconnectedException;
import com.sun.jdi.VirtualMachine;
import com.sun.jdi.event.ClassPrepareEvent;
import com.sun.jdi.event.ThreadDeathEvent;
import com.sun.jdi.request.ClassPrepareRequest;
import com.sun.jdi.request.EventRequest;
import com.sun.jdi.request.WatchpointRequest;
Expand All @@ -41,6 +45,7 @@ public class Watchpoint implements IWatchpoint, IEvaluatableBreakpoint {
private int hitCount;
private HashMap<Object, Object> propertyMap = new HashMap<>();
private Object compiledConditionalExpression = null;
private Map<Long, Object> compiledExpressions = new ConcurrentHashMap<>();

// IDebugResource
private List<EventRequest> requests = new ArrayList<>();
Expand Down Expand Up @@ -116,6 +121,7 @@ public String getCondition() {
public void setCondition(String condition) {
this.condition = condition;
setCompiledConditionalExpression(null);
compiledExpressions.clear();
}

@Override
Expand Down Expand Up @@ -147,6 +153,14 @@ public Object getProperty(Object key) {

@Override
public CompletableFuture<IWatchpoint> install() {
Disposable subscription = eventHub.events()
.filter(debugEvent -> debugEvent.event instanceof ThreadDeathEvent)
.subscribe(debugEvent -> {
ThreadReference deathThread = ((ThreadDeathEvent) debugEvent.event).thread();
compiledExpressions.remove(deathThread.uniqueID());
});
subscriptions.add(subscription);

// It's possible that different class loaders create new class with the same name.
// Here to listen to future class prepare events to handle such case.
ClassPrepareRequest classPrepareRequest = vm.eventRequestManager().createClassPrepareRequest();
Expand All @@ -155,7 +169,7 @@ public CompletableFuture<IWatchpoint> install() {
requests.add(classPrepareRequest);

CompletableFuture<IWatchpoint> future = new CompletableFuture<>();
Disposable subscription = eventHub.events()
subscription = eventHub.events()
Copy link
Contributor
@andxu andxu May 7, 2020

Choose a reason for hiding this comment

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

add subscriptions.add(subscription)?

Copy link
Contributor Author
@testforstephen testforstephen May 7, 2020

Choose a reason for hiding this comment

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

the following L182 already did that.

.filter(debugEvent -> debugEvent.event instanceof ClassPrepareEvent && (classPrepareRequest.equals(debugEvent.event.request())))
.subscribe(debugEvent -> {
ClassPrepareEvent event = (ClassPrepareEvent) debugEvent.event;
Expand Down Expand Up @@ -249,4 +263,14 @@ public void setCompiledLogpointExpression(Object compiledExpression) {
public Object getCompiledLogpointExpression() {
return null;
}

@Override
public Object getCompiledExpression(long threadId) {
return compiledExpressions.get(threadId);
}

@Override
public void setCompiledExpression(long threadId, Object compiledExpression) {
compiledExpressions.put(threadId, compiledExpression);
}
}
1 change: 1 addition & 0 deletions .../java/com/microsoft/java/debug/core/adapter/handler/AbstractDisconnectRequestHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public List<Command> getTargetCommands() {
@Override
public CompletableFuture<Response> handle(Command command, Arguments arguments, Response response,
IDebugAdapterContext context) {
context.setVmTerminated();
destroyDebugSession(command, arguments, response, context);
destroyResource(context);
return CompletableFuture.completedFuture(response);
Expand Down
15 changes: 10 additions & 5 deletions ...main/java/com/microsoft/java/debug/core/adapter/handler/SetBreakpointsRequestHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import com.sun.jdi.ReferenceType;
import com.sun.jdi.ThreadReference;
import com.sun.jdi.Value;
import com.sun.jdi.VMDisconnectedException;
import com.sun.jdi.event.BreakpointEvent;
import com.sun.jdi.event.Event;
import com.sun.jdi.event.StepEvent;
Expand Down Expand Up @@ -241,11 +242,15 @@ public static boolean handleEvaluationResult(IDebugAdapterContext context, Threa
if (resume) {
return true;
} else {
if (ex != null) {
logger.log(Level.SEVERE, String.format("[ConditionalBreakpoint]: %s", ex.getMessage() != null ? ex.getMessage() : ex.toString()), ex);
context.getProtocolServer().sendEvent(new Events.UserNotificationEvent(
Events.UserNotificationEvent.NotificationType.ERROR,
String.format("Breakpoint condition '%s' error: %s", breakpoint.getCondition(), ex.getMessage())));
if (context.isVmTerminated()) {
// do nothing
} else if (ex != null) {
if (!(ex instanceof VMDisconnectedException || ex.getCause() instanceof VMDisconnectedException)) {
logger.log(Level.SEVERE, String.format("[ConditionalBreakpoint]: %s", ex.getMessage() != null ? ex.getMessage() : ex.toString()), ex);
context.getProtocolServer().sendEvent(new Events.UserNotificationEvent(
Events.UserNotificationEvent.NotificationType.ERROR,
String.format("Breakpoint condition '%s' error: %s", breakpoint.getCondition(), ex.getMessage())));
}
} else if (value == null || resultNotBoolean) {
context.getProtocolServer().sendEvent(new Events.UserNotificationEvent(
Events.UserNotificationEvent.NotificationType.WARNING,
Expand Down
20 changes: 6 additions & 14 deletions ...in/src/main/java/com/microsoft/java/debug/plugin/internal/eval/JdtEvaluationProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,20 +148,12 @@ private CompletableFuture<Value> evaluate(String expression, ThreadReference thr
ASTEvaluationEngine engine = new ASTEvaluationEngine(project, debugTarget);
boolean newExpression = false;
if (breakpoint != null) {
if (StringUtils.isNotBlank(breakpoint.getLogMessage())) {
Copy link
Contributor
@andxu andxu May 7, 2020

Choose a reason for hiding this comment

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

why this condition is deleted: StringUtils.isNotBlank(breakpoint.getLogMessage())?

Copy link
Contributor Author
@testforstephen testforstephen May 7, 2020

Choose a reason for hiding this comment

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

this is not needed any more. Previously the compiled expression for the logpoint and conditional breakpoint are saved separately, so when evaluating a breakpoint, need check whether it's a logpoint or conditional breakpoint. In the new implementation, they are saved in a map, and fetched from the map directly.

compiledExpression = (ICompiledExpression) breakpoint.getCompiledLogpointExpression();
if (compiledExpression == null) {
newExpression = true;
compiledExpression = engine.getCompiledExpression(expression, stackframe);
breakpoint.setCompiledLogpointExpression(compiledExpression);
}
} else {
compiledExpression = (ICompiledExpression) breakpoint.getCompiledConditionalExpression();
if (compiledExpression == null) {
newExpression = true;
compiledExpression = engine.getCompiledExpression(expression, stackframe);
breakpoint.setCompiledConditionalExpression(compiledExpression);
}
long threadId = thread.uniqueID();
compiledExpression = (ICompiledExpression) breakpoint.getCompiledExpression(threadId);
if (compiledExpression == null) {
newExpression = true;
compiledExpression = engine.getCompiledExpression(expression, stackframe);
breakpoint.setCompiledExpression(threadId, compiledExpression);
}
} else {
compiledExpression = engine.getCompiledExpression(expression, stackframe);
Expand Down
0