changeset 2426:ec9be9806a79

Improve Byteman Thermostat helper. Reviewed-by: neugens Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2016-August/020636.html
author Severin Gehwolf <sgehwolf@redhat.com>
date Wed, 24 Aug 2016 12:06:35 +0200
parents 47c53031d2bc
children bc83ef2d7047
files vm-byteman/byteman-helper/src/main/java/org/jboss/byteman/thermostat/Properties.java vm-byteman/byteman-helper/src/main/java/org/jboss/byteman/thermostat/helper/ThermostatHelper.java vm-byteman/byteman-helper/src/main/java/org/jboss/byteman/thermostat/helper/transport/ipc/LocalSocketTransport.java vm-byteman/byteman-helper/src/main/java/org/jboss/byteman/thermostat/helper/transport/ipc/NoopLocalSocketChannel.java vm-byteman/byteman-helper/src/test/java/org/jboss/byteman/thermostat/helper/ThermostatHelperTest.java vm-byteman/byteman-helper/src/test/java/org/jboss/byteman/thermostat/helper/transport/ipc/LocalSocketTransportTest.java
diffstat 6 files changed, 152 insertions(+), 17 deletions(-) [+]
line wrap: on
line diff
--- a/vm-byteman/byteman-helper/src/main/java/org/jboss/byteman/thermostat/Properties.java	Tue Aug 23 19:16:09 2016 +0200
+++ b/vm-byteman/byteman-helper/src/main/java/org/jboss/byteman/thermostat/Properties.java	Wed Aug 24 12:06:35 2016 +0200
@@ -39,5 +39,6 @@
 public interface Properties {
 
     String PREFIX = "org.jboss.byteman.thermostat.";
+    String BYTEMAN_VERBOSE_PROP = "org.jboss.byteman.verbose";
 
 }
--- a/vm-byteman/byteman-helper/src/main/java/org/jboss/byteman/thermostat/helper/ThermostatHelper.java	Tue Aug 23 19:16:09 2016 +0200
+++ b/vm-byteman/byteman-helper/src/main/java/org/jboss/byteman/thermostat/helper/ThermostatHelper.java	Wed Aug 24 12:06:35 2016 +0200
@@ -53,26 +53,28 @@
     // Lock to synchronize initialization of transport between instances
     private static final Object transportLock = new Object();
     private static Transport transport = null;
-    
-    static void initTransport() {
-        transport = new TransportFactory().create();
-        Runtime.getRuntime().addShutdownHook(new Thread(new Runnable() {
-            @Override
-            public void run() {
-                transport.close();
-            }
-        }));
-    }
 
     /**
      * Constructor
      */
     protected ThermostatHelper(Rule rule) {
         super(rule);
-        // Initialize transport if not yet done
+        Runtime.getRuntime().addShutdownHook(new Thread(new Runnable() {
+            @Override
+            public void run() {
+                synchronized (transportLock) {
+                    if (transport != null) {
+                        transport.close();
+                    }
+                }
+            }
+        }));
+    }
+
+    private static void initTransport() {
         synchronized (transportLock) {
             if (transport == null) {
-                initTransport();
+                transport = new TransportFactory().create();
             }
         }
     }
@@ -109,14 +111,33 @@
     public void send(String marker, Object... dataArray) {
         LinkedHashMap<String, Object> data = toMap(dataArray);
         BytemanMetric rec = new BytemanMetric(marker, data);
-        transport.send(rec);
+        synchronized (transportLock) {
+            transport.send(rec);
+        }
     }
     
-    // For testing purposes
     static void setTransport(Transport transport) {
         synchronized (transportLock) {
             ThermostatHelper.transport = transport;
         }
     }
+    
+    // package-private for testing
+    static Transport getTransport() {
+        return transport;
+    }
+    
+    // Called by Byteman on helper de-activation
+    public static void deactivated() {
+        synchronized (transportLock) {
+            transport.close();
+        }
+        setTransport(null);
+    }
+    
+    // Called by Byteman on helper activation
+    public static void activated() {
+        initTransport();
+    }
 
 }
--- a/vm-byteman/byteman-helper/src/main/java/org/jboss/byteman/thermostat/helper/transport/ipc/LocalSocketTransport.java	Tue Aug 23 19:16:09 2016 +0200
+++ b/vm-byteman/byteman-helper/src/main/java/org/jboss/byteman/thermostat/helper/transport/ipc/LocalSocketTransport.java	Wed Aug 24 12:06:35 2016 +0200
@@ -45,7 +45,6 @@
 
 import org.jboss.byteman.thermostat.helper.BytemanMetric;
 import org.jboss.byteman.thermostat.helper.Transport;
-import org.jboss.byteman.thermostat.helper.TransportException;
 import org.jboss.byteman.thermostat.helper.Utils;
 
 /**
@@ -79,11 +78,17 @@
         this.attempts = attempts;
         this.breakIntervalMillis = breakIntervalMillis;
         this.socketName = socketName;
+        LocalSocketChannel ch;
         try {
-            this.channel = channelFactory.open(ipcConfig, socketName);
+            ch = channelFactory.open(ipcConfig, socketName);
         } catch (Exception e) {
-            throw new TransportException("Error opening Thermostat socket: [" + socketName + "]", e);
+            // Cannot write to thermostat endpoint for some reason.
+            // Use a no-op channel so that we keep things reasonable in the
+            // target JVM
+            String reason = "Error opening Thermostat socket: [" + socketName + "]";
+            ch = new NoopLocalSocketChannel(reason);
         }
+        this.channel = ch;
     }
 
     /**
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/vm-byteman/byteman-helper/src/main/java/org/jboss/byteman/thermostat/helper/transport/ipc/NoopLocalSocketChannel.java	Wed Aug 24 12:06:35 2016 +0200
@@ -0,0 +1,65 @@
+/*
+ * Copyright 2012-2016 Red Hat, Inc.
+ *
+ * This file is part of Thermostat.
+ *
+ * Thermostat is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published
+ * by the Free Software Foundation; either version 2, or (at your
+ * option) any later version.
+ *
+ * Thermostat is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Thermostat; see the file COPYING.  If not see
+ * <http://www.gnu.org/licenses/>.
+ *
+ * Linking this code with other modules is making a combined work
+ * based on this code.  Thus, the terms and conditions of the GNU
+ * General Public License cover the whole combination.
+ *
+ * As a special exception, the copyright holders of this code give
+ * you permission to link this code with independent modules to
+ * produce an executable, regardless of the license terms of these
+ * independent modules, and to copy and distribute the resulting
+ * executable under terms of your choice, provided that you also
+ * meet, for each linked independent module, the terms and conditions
+ * of the license of that module.  An independent module is a module
+ * which is not derived from or based on this code.  If you modify
+ * this code, you may extend this exception to your version of the
+ * library, but you are not obligated to do so.  If you do not wish
+ * to do so, delete this exception statement from your version.
+ */
+
+package org.jboss.byteman.thermostat.helper.transport.ipc;
+
+import java.io.IOException;
+import java.nio.ByteBuffer;
+
+import org.jboss.byteman.thermostat.Properties;
+
+public class NoopLocalSocketChannel implements LocalSocketChannel {
+    
+    private static final boolean IS_VERBOSE = Boolean.getBoolean(Properties.BYTEMAN_VERBOSE_PROP);
+    private final String reason;
+
+    public NoopLocalSocketChannel(String reason) {
+        this.reason = reason;
+    }
+    
+    @Override
+    public void close() throws IOException {
+        // nothing
+    }
+
+    @Override
+    public void write(ByteBuffer buffer) throws IOException {
+        if (IS_VERBOSE) {
+            System.out.println("[" + NoopLocalSocketChannel.class.getSimpleName() + "] Discarding message received via send(). Reason: " + reason);
+        }
+    }
+
+}
--- a/vm-byteman/byteman-helper/src/test/java/org/jboss/byteman/thermostat/helper/ThermostatHelperTest.java	Tue Aug 23 19:16:09 2016 +0200
+++ b/vm-byteman/byteman-helper/src/test/java/org/jboss/byteman/thermostat/helper/ThermostatHelperTest.java	Wed Aug 24 12:06:35 2016 +0200
@@ -38,10 +38,13 @@
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
 
 import java.io.ByteArrayOutputStream;
 import java.io.PrintStream;
@@ -178,6 +181,33 @@
             System.setOut(oldOut);
         }
     }
+    
+    @Test
+    public void testActivatedNonNull() {
+        Transport t = mock(Transport.class);
+        ThermostatHelper.setTransport(t);
+        ThermostatHelper.activated();
+        assertSame(t, ThermostatHelper.getTransport());
+    }
+    
+    @Test
+    public void testConstructorDoesNotSetTransport() {
+        ThermostatHelper.setTransport(null);
+        new ThermostatHelper(mock(Rule.class));
+        assertNull(ThermostatHelper.getTransport());
+    }
+    
+    @Test
+    public void testDeactivated() {
+        Transport t = mock(Transport.class);
+        ThermostatHelper.setTransport(t);
+        assertSame(t, ThermostatHelper.getTransport());
+        
+        ThermostatHelper.deactivated();
+        verify(t).close();
+        verifyNoMoreInteractions(t);
+        assertNull(ThermostatHelper.getTransport());
+    }
 
     private Object getValue(boolean stringValue, int i) {
         if (stringValue) {
--- a/vm-byteman/byteman-helper/src/test/java/org/jboss/byteman/thermostat/helper/transport/ipc/LocalSocketTransportTest.java	Tue Aug 23 19:16:09 2016 +0200
+++ b/vm-byteman/byteman-helper/src/test/java/org/jboss/byteman/thermostat/helper/transport/ipc/LocalSocketTransportTest.java	Wed Aug 24 12:06:35 2016 +0200
@@ -37,6 +37,7 @@
 package org.jboss.byteman.thermostat.helper.transport.ipc;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.mock;
@@ -79,6 +80,18 @@
         t.close();
         verify(factory).open(any(File.class), eq("foo-name"));
     }
+    
+    @SuppressWarnings("unchecked")
+    @Test
+    public void instantiationFailureOfChannelOpeningUsesNoopChannelInstead() {
+        LocalSocketTransport t = null;
+        LocalSocketChannelFactory failFactory = mock(LocalSocketChannelFactory.class);
+        when(failFactory.open(any(File.class), any(String.class))).thenThrow(RuntimeException.class);
+        t = new LocalSocketTransport(0, 0, mock(File.class), "foo-name", 10, 1, 100, failFactory);
+        LocalSocketChannel channel = t.getChannel();
+        assertTrue(channel instanceof NoopLocalSocketChannel);
+        t.close();
+    }
 
     @Test
     public void sendWritesToChannelInBatches() throws InterruptedException, IOException {