changeset 28:49f6575169ce

JMC-5398: Call to "attach busy" JVM hangs and blocks the listing of other local JVMs Summary: Add timeouts to ensure connecting to busy or blocked JVMs does not block forever Reviewed-by: neugens, hirt Contributed-by: Joshua Matsuoka <jmatsuok@redhat.com>
author neugens
date Tue, 31 Jul 2018 20:02:30 +0200
parents f9987e62b984
children b908f78e9f9d
files application/org.openjdk.jmc.browser.attach/src/main/java/org/openjdk/jmc/browser/attach/LocalConnectionDescriptor.java application/org.openjdk.jmc.browser.attach/src/main/java/org/openjdk/jmc/browser/attach/LocalJVMToolkit.java
diffstat 2 files changed, 218 insertions(+), 159 deletions(-) [+]
line wrap: on
line diff
--- a/application/org.openjdk.jmc.browser.attach/src/main/java/org/openjdk/jmc/browser/attach/LocalConnectionDescriptor.java	Tue Jul 31 12:57:49 2018 +0200
+++ b/application/org.openjdk.jmc.browser.attach/src/main/java/org/openjdk/jmc/browser/attach/LocalConnectionDescriptor.java	Tue Jul 31 20:02:30 2018 +0200
@@ -38,6 +38,12 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Properties;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
 import java.util.logging.Level;
 
 import javax.management.remote.JMXServiceURL;
@@ -56,9 +62,11 @@
 public class LocalConnectionDescriptor implements IConnectionDescriptor {
 
 	private static final String SELF_HOST_NAME = "localhost"; //$NON-NLS-1$
+	private static final String ATTACH_TIMED_OUT_ERROR_MESSAGE = "Timed out attempting to attach to target JVM!"; //$NON-NLS-1$
 	private static final String COULD_NOT_RETRIEVE_URL_ERROR_MESSAGE = "Could not retrieve the in-memory service URL after starting the in-memory agent!"; //$NON-NLS-1$
 	private final boolean isAutoStartAgent = fetchAutoStartAgentFromStore();
 	private final int pid;
+	private static final int TIMEOUT_THRESHOLD = 5;
 	private final boolean attachable;
 	private JMXServiceURL url;
 
@@ -99,27 +107,37 @@
 	private void tryAgentLoadingStyleOfStartingTheAgent(String pid) throws LazyServiceURLResolveException {
 		// This is hotspot style starting of the agent. We do this
 		// pretty much the way JConsole does it.
-		VirtualMachine vm = null;
 		try {
-			vm = VirtualMachine.attach(pid);
-			String home = vm.getSystemProperties().getProperty("java.home"); //$NON-NLS-1$
-			// Normally in ${java.home}/jre/lib/management-agent.jar but might
-			// be in ${java.home}/lib in build environments.
-			String agent = home + File.separator + "jre" + File.separator + "lib" + File.separator //$NON-NLS-1$ //$NON-NLS-2$
-					+ "management-agent.jar"; //$NON-NLS-1$
-			File f = new File(agent);
-			if (!f.exists()) {
-				agent = home + File.separator + "lib" + File.separator + "management-agent.jar"; //$NON-NLS-1$ //$NON-NLS-2$
-				f = new File(agent);
-				if (!f.exists()) {
-					throw new LazyServiceURLResolveException("Management agent not found"); //$NON-NLS-1$
+			// Add a timeout here so we don't block forever if the JVM is busy/suspended. See JMC-5398
+			ExecutorService service = Executors.newSingleThreadExecutor();
+			Future<Void> future = service.submit(new Callable<Void>() {
+				@Override
+				public Void call() throws Exception {
+					VirtualMachine vm = VirtualMachine.attach(pid);
+					String home = vm.getSystemProperties().getProperty("java.home"); //$NON-NLS-1$
+					// Normally in ${java.home}/jre/lib/management-agent.jar but might
+					// be in ${java.home}/lib in build environments.
+					String agent = home + File.separator + "jre" + File.separator + "lib" + File.separator //$NON-NLS-1$ //$NON-NLS-2$
+							+ "management-agent.jar"; //$NON-NLS-1$
+					File f = new File(agent);
+					if (!f.exists()) {
+						agent = home + File.separator + "lib" + File.separator + "management-agent.jar"; //$NON-NLS-1$ //$NON-NLS-2$
+						f = new File(agent);
+						if (!f.exists()) {
+							throw new LazyServiceURLResolveException("Management agent not found"); //$NON-NLS-1$
+						}
+					}
+					agent = f.getCanonicalPath();
+					vm.loadAgent(agent, "com.sun.management.jmxremote"); //$NON-NLS-1$
+					Properties agentProps = vm.getAgentProperties();
+					setAddress((String) agentProps.get(LocalJVMToolkit.LOCAL_CONNECTOR_ADDRESS_PROP));
+					vm.detach();
+					return null;
 				}
-			}
-			agent = f.getCanonicalPath();
-			vm.loadAgent(agent, "com.sun.management.jmxremote"); //$NON-NLS-1$
-			Properties agentProps = vm.getAgentProperties();
-			setAddress((String) agentProps.get(LocalJVMToolkit.LOCAL_CONNECTOR_ADDRESS_PROP));
-			vm.detach();
+			});
+			future.get(TIMEOUT_THRESHOLD, TimeUnit.SECONDS);
+		} catch (TimeoutException t) {
+			throw new LazyServiceURLResolveException(ATTACH_TIMED_OUT_ERROR_MESSAGE, t);
 		} catch (Exception x) {
 			LazyServiceURLResolveException lsure = new LazyServiceURLResolveException(
 					"Attach not supported for the JVM with PID " + pid //$NON-NLS-1$
@@ -158,23 +176,38 @@
 	 * @throws AttachNotSupportedException
 	 */
 	private void tryJCMDStyleStartingOfTheAgent(String name)
-			throws AttachNotSupportedException, IOException, AgentLoadException {
-		VirtualMachine vm = null;
+			throws IOException, AgentLoadException {
 		try {
-			// First try getting some versioning information
-			vm = VirtualMachine.attach(name);
-			LocalJVMToolkit.executeCommandForPid(vm, name, "ManagementAgent.start_local"); //$NON-NLS-1$
-			// Get in memory Service URL...
-			JMXServiceURL inMemURL = LocalJVMToolkit.getInMemoryURLFromPID(Integer.parseInt(name));
-			if (inMemURL == null) {
-				BrowserAttachPlugin.getPluginLogger().log(Level.SEVERE, COULD_NOT_RETRIEVE_URL_ERROR_MESSAGE);
-				throw new LazyServiceURLResolveException(COULD_NOT_RETRIEVE_URL_ERROR_MESSAGE);
-			}
-			url = inMemURL;
-		} finally {
-			if (vm != null) {
-				vm.detach();
-			}
+			// Enforce a timeout here to ensure we don't block forever if the JVM is busy/suspended. See JMC-5398
+			ExecutorService service = Executors.newSingleThreadExecutor();
+			Future<Void> future = service.submit(new Callable<Void>() {
+				@Override
+				public Void call() throws Exception {
+					VirtualMachine vm = null;
+					try {
+						// First try getting some versioning information
+						vm = VirtualMachine.attach(name);
+						LocalJVMToolkit.executeCommandForPid(vm, name, "ManagementAgent.start_local"); //$NON-NLS-1$
+						// Get in memory Service URL...
+						JMXServiceURL inMemURL = LocalJVMToolkit.getInMemoryURLFromPID(Integer.parseInt(name));
+						if (inMemURL == null) {
+							BrowserAttachPlugin.getPluginLogger().log(Level.SEVERE, COULD_NOT_RETRIEVE_URL_ERROR_MESSAGE);
+							throw new LazyServiceURLResolveException(COULD_NOT_RETRIEVE_URL_ERROR_MESSAGE);
+						}
+						url = inMemURL;
+					} finally {
+						if (vm != null) {
+							vm.detach();
+						}
+					}
+					return null;
+				}
+			});
+			future.get(TIMEOUT_THRESHOLD, TimeUnit.SECONDS);
+		} catch (TimeoutException t) {
+			throw new LazyServiceURLResolveException(ATTACH_TIMED_OUT_ERROR_MESSAGE, t);
+		} catch (Exception e) {
+			throw new LazyServiceURLResolveException(COULD_NOT_RETRIEVE_URL_ERROR_MESSAGE);
 		}
 	}
 
--- a/application/org.openjdk.jmc.browser.attach/src/main/java/org/openjdk/jmc/browser/attach/LocalJVMToolkit.java	Tue Jul 31 12:57:49 2018 +0200
+++ b/application/org.openjdk.jmc.browser.attach/src/main/java/org/openjdk/jmc/browser/attach/LocalJVMToolkit.java	Tue Jul 31 20:02:30 2018 +0200
@@ -46,6 +46,11 @@
 import java.util.Properties;
 import java.util.Set;
 import java.util.WeakHashMap;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
 import java.util.logging.Level;
 
 import javax.management.remote.JMXServiceURL;
@@ -111,6 +116,8 @@
 	static final String JVM_ARGS_PROP = "sun.jvm.args"; //$NON-NLS-1$
 	static final String JVM_FLAGS_PROP = "sun.jvm.flags"; //$NON-NLS-1$
 	static final String JAVA_COMMAND_PROP = "sun.java.command"; //$NON-NLS-1$
+	
+	private static final int TIMEOUT_THRESHOLD = 5;
 
 	private LocalJVMToolkit() {
 		// Toolkit
@@ -167,7 +174,7 @@
 					connDesc = createMonitoredJvmDescriptor(host, (Integer) vmid);
 				}
 
-				if (includeUnconnectables
+				if ((includeUnconnectables && connDesc != null)
 						|| (connDesc != null && !connDesc.getServerDescriptor().getJvmInfo().isUnconnectable())) {
 					map.put(vmid, connDesc);
 				}
@@ -176,79 +183,92 @@
 	}
 
 	private static DiscoveryEntry createMonitoredJvmDescriptor(MonitoredHost host, Integer vmid) {
-		DiscoveryEntry connDesc;
-		int pid = vmid.intValue();
-		String name = vmid.toString(); // default to pid if name not available
-		Connectable connectable = NO;
-		JVMType type = JVMType.OTHER;
-		JVMArch jvmArch = JVMArch.OTHER;
-		boolean isDebug = false;
-		String address = null;
-		String version = null;
-		String jvmArgs = null;
 		try {
-			// This used to leak one \BaseNamedObjects\hsperfdata_* Section handle on Windows
-			MonitoredVm mvm = host.getMonitoredVm(new VmIdentifier(name));
-			try {
-				// use the command line as the display name
-				name = MonitoredVmUtil.commandLine(mvm);
-				jvmArgs = MonitoredVmUtil.jvmArgs(mvm);
-				StringMonitor sm = (StringMonitor) mvm.findByName("java.property.java.vm.name"); //$NON-NLS-1$
-				if (sm != null) {
-					type = getJVMType(sm.stringValue());
+			// Enforce a timeout here to make sure we don't block forever if the JVM is busy/suspended. See JMC-5398
+			ExecutorService service = Executors.newSingleThreadExecutor();
+			Future<DiscoveryEntry> future = service.submit(new Callable<DiscoveryEntry>() {
+				@Override
+				public DiscoveryEntry call() throws Exception {
+					DiscoveryEntry connDesc;
+					int pid = vmid.intValue();
+					String name = vmid.toString(); // default to pid if name not available
+					Connectable connectable = NO;
+					JVMType type = JVMType.OTHER;
+					JVMArch jvmArch = JVMArch.OTHER;
+					boolean isDebug = false;
+					String address = null;
+					String version = null;
+					String jvmArgs = null;
+					try {
+						// This used to leak one \BaseNamedObjects\hsperfdata_* Section handle on Windows
+						MonitoredVm mvm = host.getMonitoredVm(new VmIdentifier(name));
+						try {
+							// use the command line as the display name
+							name = MonitoredVmUtil.commandLine(mvm);
+							jvmArgs = MonitoredVmUtil.jvmArgs(mvm);
+							StringMonitor sm = (StringMonitor) mvm.findByName("java.property.java.vm.name"); //$NON-NLS-1$
+							if (sm != null) {
+								type = getJVMType(sm.stringValue());
+							}
+
+							sm = (StringMonitor) mvm.findByName("java.property.java.version"); //$NON-NLS-1$
+							if (sm != null) {
+								version = sm.stringValue();
+							}
+
+							if (version == null) {
+								// Use java.vm.version when java.version is not exposed as perfcounter (HotSpot 1.5 and JRockit)
+								sm = (StringMonitor) mvm.findByName("java.property.java.vm.version"); //$NON-NLS-1$
+								if (sm != null) {
+									String vmVersion = sm.stringValue();
+									if (type == JVMType.JROCKIT) {
+										version = JavaVMVersionToolkit.decodeJavaVersion(vmVersion);
+									} else {
+										version = JavaVMVersionToolkit.parseJavaVersion(vmVersion);
+									}
+								}
+							}
+							if (version == null) {
+								version = "0"; //$NON-NLS-1$
+							}
+
+							if (sm != null) {
+								isDebug = isDebug(sm.stringValue());
+							}
+							// NOTE: isAttachable seems to return true even if a real attach is not possible.
+							// attachable = MonitoredVmUtil.isAttachable(mvm);
+
+							jvmArch = getArch(vmid);
+							// Check if the in-memory agent has been started, in that case we can connect anyway
+							JMXServiceURL inMemURL = null;
+							try {
+								inMemURL = LocalJVMToolkit.getInMemoryURLFromPID(vmid);
+							} catch (IOException e) {
+								BrowserAttachPlugin.getPluginLogger().log(Level.WARNING,
+										"Got exception when trying to get in-memory url for jvm with PID " + vmid, e); //$NON-NLS-1$
+							}
+							if (inMemURL != null) {
+								connectable = MGMNT_AGENT_STARTED;
+							}
+
+							// This used to leak one \BaseNamedObjects\hsperfdata_* Section handle on Windows
+							address = AttachToolkit.importFromPid(pid);
+						} finally {
+							// Although the current implementation of LocalMonitoredVm for Windows doesn't do much here, we should always call detach.
+							mvm.detach();
+						}
+					} catch (Exception x) {
+						// ignore
+					}
+					connDesc = createDescriptor(name, jvmArgs, vmid, connectable, type, jvmArch, address, version, isDebug);
+					return connDesc;
 				}
-
-				sm = (StringMonitor) mvm.findByName("java.property.java.version"); //$NON-NLS-1$
-				if (sm != null) {
-					version = sm.stringValue();
-				}
-
-				if (version == null) {
-					// Use java.vm.version when java.version is not exposed as perfcounter (HotSpot 1.5 and JRockit)
-					sm = (StringMonitor) mvm.findByName("java.property.java.vm.version"); //$NON-NLS-1$
-					if (sm != null) {
-						String vmVersion = sm.stringValue();
-						if (type == JVMType.JROCKIT) {
-							version = JavaVMVersionToolkit.decodeJavaVersion(vmVersion);
-						} else {
-							version = JavaVMVersionToolkit.parseJavaVersion(vmVersion);
-						}
-					}
-				}
-				if (version == null) {
-					version = "0"; //$NON-NLS-1$
-				}
-
-				if (sm != null) {
-					isDebug = isDebug(sm.stringValue());
-				}
-				// NOTE: isAttachable seems to return true even if a real attach is not possible.
-				// attachable = MonitoredVmUtil.isAttachable(mvm);
-
-				jvmArch = getArch(vmid);
-				// Check if the in-memory agent has been started, in that case we can connect anyway
-				JMXServiceURL inMemURL = null;
-				try {
-					inMemURL = LocalJVMToolkit.getInMemoryURLFromPID(vmid);
-				} catch (IOException e) {
-					BrowserAttachPlugin.getPluginLogger().log(Level.WARNING,
-							"Got exception when trying to get in-memory url for jvm with PID " + vmid, e); //$NON-NLS-1$
-				}
-				if (inMemURL != null) {
-					connectable = MGMNT_AGENT_STARTED;
-				}
-
-				// This used to leak one \BaseNamedObjects\hsperfdata_* Section handle on Windows
-				address = AttachToolkit.importFromPid(pid);
-			} finally {
-				// Although the current implementation of LocalMonitoredVm for Windows doesn't do much here, we should always call detach.
-				mvm.detach();
-			}
-		} catch (Exception x) {
-			// ignore
+			});
+			return future.get(TIMEOUT_THRESHOLD, TimeUnit.SECONDS);
+		} catch (Exception e) {
+			BrowserAttachPlugin.getPluginLogger().log(Level.WARNING, "Failed to create descriptor for jvm with PID " + vmid, e);
+			return null;
 		}
-		connDesc = createDescriptor(name, jvmArgs, vmid, connectable, type, jvmArch, address, version, isDebug);
-		return connDesc;
 	}
 
 	/*
@@ -314,7 +334,7 @@
 
 					if (connDesc != null && !connDesc.getServerDescriptor().getJvmInfo().isUnconnectable()) {
 						map.put(vmid, connDesc);
-					}
+					} 
 				}
 			} catch (NumberFormatException e) {
 				// do not support vmid different than pid
@@ -323,53 +343,65 @@
 	}
 
 	private static DiscoveryEntry createAttachableJvmDescriptor(VirtualMachineDescriptor vmd) {
-		DiscoveryEntry connDesc = null;
-		Connectable connectable;
-		boolean isDebug = false;
-		JVMType jvmType = JVMType.OTHER;
-		JVMArch jvmArch = JVMArch.OTHER;
-		String address = null;
-		String version = null;
-		String javaArgs = null;
-		String jvmArgs = null;
-		String jvmVersion = null;
-
 		try {
-			// Attach creates one process handle on Windows.
-			// This leaks one thread handle due to Sun bug in j2se/src/windows/native/sun/tools/attach/WindowsVirtualMachine.c
-			VirtualMachine vm = VirtualMachine.attach(vmd);
-			try {
-				connectable = ATTACHABLE;
-				// This leaks one thread handle due to Sun bug in j2se/src/windows/native/sun/tools/attach/WindowsVirtualMachine.c
-				Properties props = null;
-				try {
-					props = vm.getSystemProperties();
-				} catch (IOException e) {
-					BrowserAttachPlugin.getPluginLogger().log(Level.FINER,
-							"Got the following exception message when getting system properties from vm with PID " //$NON-NLS-1$
-									+ vmd + ": " + e.getMessage()); //$NON-NLS-1$
-
-				}
-				if (props != null) {
-					String vmName = props.getProperty("java.vm.name"); //$NON-NLS-1$
-					jvmType = getJVMType(vmName);
-					version = props.getProperty("java.version"); //$NON-NLS-1$
-					jvmVersion = props.getProperty("java.vm.version"); //$NON-NLS-1$
-					isDebug = isDebug(jvmVersion);
-					jvmArch = JVMArch.getJVMArch(props);
-				}
-				Properties agentProps = vm.getAgentProperties();
-				address = (String) agentProps.get(LOCAL_CONNECTOR_ADDRESS_PROP);
-				javaArgs = resolveCommandLine(vm, vmd.displayName(), props, agentProps);
-				jvmArgs = (String) agentProps.get("sun.jvm.args"); //$NON-NLS-1$
-
-			} finally {
-				// Always detach. Releases one process handle on Windows.
-				vm.detach();
-			}
-		} catch (AttachNotSupportedException x) {
-			// Not attachable
-			connectable = NO;
+			// Enforce a timeout here to ensure we don't block forever if the JVM is busy or suspended. See JMC-5398.
+			 ExecutorService service = Executors.newSingleThreadExecutor();
+			 Future<DiscoveryEntry> future = service.submit(new Callable<DiscoveryEntry>() {
+				 @Override
+				 public DiscoveryEntry call() throws Exception {
+					DiscoveryEntry connDesc = null;
+					Connectable connectable;
+					boolean isDebug = false;
+					JVMType jvmType = JVMType.OTHER;
+					JVMArch jvmArch = JVMArch.OTHER;
+					String address = null;
+					String version = null;
+					String javaArgs = null;
+					String jvmArgs = null;
+					String jvmVersion = null;
+					VirtualMachine vm = null;
+					try {
+						// Attach creates one process handle on Windows.
+						// This leaks one thread handle due to Sun bug in j2se/src/windows/native/sun/tools/attach/WindowsVirtualMachine.c
+						vm = VirtualMachine.attach(vmd);
+						connectable = ATTACHABLE;
+						// This leaks one thread handle due to Sun bug in j2se/src/windows/native/sun/tools/attach/WindowsVirtualMachine.c
+						Properties props = null;
+						try {
+							props = vm.getSystemProperties();
+						} catch (IOException e) {
+							BrowserAttachPlugin.getPluginLogger().log(Level.FINER,
+									"Got the following exception message when getting system properties from vm with PID " //$NON-NLS-1$
+											+ vmd + ": " + e.getMessage()); //$NON-NLS-1$
+						}
+						if (props != null) {
+							String vmName = props.getProperty("java.vm.name"); //$NON-NLS-1$
+							jvmType = getJVMType(vmName);
+							version = props.getProperty("java.version"); //$NON-NLS-1$
+							jvmVersion = props.getProperty("java.vm.version"); //$NON-NLS-1$
+							isDebug = isDebug(jvmVersion);
+							jvmArch = JVMArch.getJVMArch(props);
+						}
+						Properties agentProps = vm.getAgentProperties();
+						address = (String) agentProps.get(LOCAL_CONNECTOR_ADDRESS_PROP);
+						javaArgs = resolveCommandLine(vm, vmd.displayName(), props, agentProps);
+						jvmArgs = (String) agentProps.get("sun.jvm.args"); //$NON-NLS-1$
+					} catch (AttachNotSupportedException x) {
+						// Not attachable
+						connectable = NO;
+					} finally {
+						// Always detach. Releases one process handle on Windows.
+						vm.detach();
+					}
+					if (connectable.isAttachable()) {
+						connDesc = createDescriptor(javaArgs, jvmArgs, Integer.parseInt(vmd.id()), connectable, jvmType, jvmArch,
+								address, version, isDebug);
+					}
+					BrowserAttachPlugin.getPluginLogger().info("Done resolving PID " + vmd); //$NON-NLS-1$
+					return connDesc;
+				 }
+			 });
+			 return future.get(TIMEOUT_THRESHOLD, TimeUnit.SECONDS);
 		} catch (Throwable t) {
 			// Serious problem for this JVM, let's skip this one.
 			if (!isErrorMessageSent) {
@@ -382,12 +414,6 @@
 			}
 			return null;
 		}
-		if (connectable.isAttachable()) {
-			connDesc = createDescriptor(javaArgs, jvmArgs, Integer.parseInt(vmd.id()), connectable, jvmType, jvmArch,
-					address, version, isDebug);
-		}
-		BrowserAttachPlugin.getPluginLogger().info("Done resolving PID " + vmd); //$NON-NLS-1$
-		return connDesc;
 	}
 
 	private static MonitoredHost getMonitoredHost() {