From dacf4403ecd114badee018b07c01b03d0e5f1ab5 Mon Sep 17 00:00:00 2001 From: Jinbo Wang Date: Thu, 7 May 2020 16:09:45 +0800 Subject: [PATCH] Fix race competition when the breakpoint needs be evaluated in multi threads (#325) * Fix race competition when the breakpoint needs be evaluated in multi threads Signed-off-by: Jinbo Wang * Use thread-safe map Signed-off-by: Jinbo Wang --- .../debug/core/EvaluatableBreakpoint.java | 48 +++++++++++++++++-- .../debug/core/IEvaluatableBreakpoint.java | 33 +++++++++++++ .../microsoft/java/debug/core/Watchpoint.java | 26 +++++++++- .../AbstractDisconnectRequestHandler.java | 1 + .../handler/SetBreakpointsRequestHandler.java | 15 ++++-- .../internal/eval/JdtEvaluationProvider.java | 20 +++----- 6 files changed, 118 insertions(+), 25 deletions(-) diff --git a/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/EvaluatableBreakpoint.java b/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/EvaluatableBreakpoint.java index ee1f36e4a..1a0647411 100644 --- a/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/EvaluatableBreakpoint.java +++ b/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/EvaluatableBreakpoint.java @@ -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 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 @@ -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 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(); } } diff --git a/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/IEvaluatableBreakpoint.java b/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/IEvaluatableBreakpoint.java index 6d465b7c2..16d8904c9 100644 --- a/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/IEvaluatableBreakpoint.java +++ b/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/IEvaluatableBreakpoint.java @@ -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 + * null. + * + * @param threadId thread the breakpoint was hit in + * @return compiled expression or null + */ + Object getCompiledExpression(long threadId); } diff --git a/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/Watchpoint.java b/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/Watchpoint.java index 18b4314cf..3de321ec8 100644 --- a/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/Watchpoint.java +++ b/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/Watchpoint.java @@ -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; @@ -41,6 +45,7 @@ public class Watchpoint implements IWatchpoint, IEvaluatableBreakpoint { private int hitCount; private HashMap propertyMap = new HashMap<>(); private Object compiledConditionalExpression = null; + private Map compiledExpressions = new ConcurrentHashMap<>(); // IDebugResource private List requests = new ArrayList<>(); @@ -116,6 +121,7 @@ public String getCondition() { public void setCondition(String condition) { this.condition = condition; setCompiledConditionalExpression(null); + compiledExpressions.clear(); } @Override @@ -147,6 +153,14 @@ public Object getProperty(Object key) { @Override public CompletableFuture 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(); @@ -155,7 +169,7 @@ public CompletableFuture install() { requests.add(classPrepareRequest); CompletableFuture future = new CompletableFuture<>(); - Disposable subscription = eventHub.events() + subscription = eventHub.events() .filter(debugEvent -> debugEvent.event instanceof ClassPrepareEvent && (classPrepareRequest.equals(debugEvent.event.request()))) .subscribe(debugEvent -> { ClassPrepareEvent event = (ClassPrepareEvent) debugEvent.event; @@ -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); + } } diff --git a/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/adapter/handler/AbstractDisconnectRequestHandler.java b/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/adapter/handler/AbstractDisconnectRequestHandler.java index c94c1e9d0..fd5c68d6c 100644 --- a/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/adapter/handler/AbstractDisconnectRequestHandler.java +++ b/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/adapter/handler/AbstractDisconnectRequestHandler.java @@ -40,6 +40,7 @@ public List getTargetCommands() { @Override public CompletableFuture handle(Command command, Arguments arguments, Response response, IDebugAdapterContext context) { + context.setVmTerminated(); destroyDebugSession(command, arguments, response, context); destroyResource(context); return CompletableFuture.completedFuture(response); diff --git a/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/adapter/handler/SetBreakpointsRequestHandler.java b/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/adapter/handler/SetBreakpointsRequestHandler.java index 23ed69213..fe246a61e 100644 --- a/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/adapter/handler/SetBreakpointsRequestHandler.java +++ b/com.microsoft.java.debug.core/src/main/java/com/microsoft/java/debug/core/adapter/handler/SetBreakpointsRequestHandler.java @@ -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; @@ -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, diff --git a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/eval/JdtEvaluationProvider.java b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/eval/JdtEvaluationProvider.java index ceb93c5cf..21e2dff6e 100644 --- a/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/eval/JdtEvaluationProvider.java +++ b/com.microsoft.java.debug.plugin/src/main/java/com/microsoft/java/debug/plugin/internal/eval/JdtEvaluationProvider.java @@ -148,20 +148,12 @@ private CompletableFuture evaluate(String expression, ThreadReference thr ASTEvaluationEngine engine = new ASTEvaluationEngine(project, debugTarget); boolean newExpression = false; if (breakpoint != null) { - if (StringUtils.isNotBlank(breakpoint.getLogMessage())) { - 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);