Refactor HttpURLConnection to use a ScheduledThreadPoolExecutor

instrumentation/httpurlconnection/README.md

@@ -15,7 +15,7 @@ Calling `addOutboundRequestHeaders` will result in a span (which could be genera
 Much of this complication is due to the fact that once the `connect` method has been called, the `HttpURLConnection` header map can no longer be updated and any calls to `addOutboundRequestHeaders` will fail. This means that if `connect` is explicitly called first we have no choice but to call `addOutboundRequestHeaders` at that point and try to predict what sequence of events happens next and whether an external call should be recorded and, if so, how to call `reportAsExternal` on the same tracer/segment so that only a single external span gets generated.
 
 Behavior expected when `HttpURLConnection` APIs are called in different combos and orders.
-* If only `connect` is called, then NO request is made over the wire and NO external call is reported. The instrumentation starts a `TimerTask` if `connect` is called first, waits for a set period of time to determine if any further `HttpURLConnection` APIs are called before deciding how to proceed. If no other API is called, then the segment is just ignored and no external is reported. If any other method is called an external call will be recorded.
+* If only `connect` is called, then NO request is made over the wire and NO external call is reported. The instrumentation starts a `SegmentCleanupTask` if `connect` is called first, waits for a set period of time to determine if any further `HttpURLConnection` APIs are called before deciding how to proceed. If no other API is called, then the segment is just ignored and no external is reported. If any other method is called an external call will be recorded.
 * Calling any of `getOutputStream`, `getInputStream`, `getResponseCode`, or `getHeaderFields` all result in an external call being recorded.
 
 ## Example HttpURLConnection usage

instrumentation/httpurlconnection/src/main/java/com/nr/agent/instrumentation/httpurlconnection/MetricState.java

@@ -17,8 +17,10 @@
 
 import java.net.HttpURLConnection;
 import java.net.URI;
-import java.util.Timer;
-import java.util.TimerTask;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
 
 public class MetricState {
     private static final String LIBRARY = "HttpURLConnection";
@@ -38,6 +40,9 @@ public class MetricState {
     // segment used to track timing of external request, add addOutboundRequestHeaders, and reportAsExternal
     private Segment segment;
 
+    private static final ScheduledThreadPoolExecutor threadPool = (ScheduledThreadPoolExecutor) Executors.newScheduledThreadPool(5);
+    private ScheduledFuture<?> segmentCleanupTaskFuture;
+
     /**
      * Start Segment timing when the first HttpURLConnection API method is invoked.
      * The Segment timing should end when a request has taken place and an external call has been recorded.
@@ -54,50 +59,65 @@ private void startSegmentIfNull(Transaction tx, String operation) {
 
     /**
      * Keep track of which HttpURLConnection API method was most recently called.
-     * If connect was called first, then start a TimerTask to potentially ignore the segment
+     * If connect was called first, then start a cleanup task to potentially ignore the segment
      * if it is determined that no other method was called after it.
      *
      * @param operation HttpURLConnection method being invoked
      */
     private void handleSegmentsForNonNetworkMethods(String operation) {
         if (operation.equals(CONNECT_OP)) {
-            // Only ever start the TimerTask if connect is the first method called
+            // Only ever start the cleanup task if connect is the first method called
             if (lastOperation == null) {
                 lastOperation = operation;
-                startSegmentExpirationTimerTask();
+                startSegmentCleanupTask();
             }
         } else {
             networkRequestMethodCalled = true;
+            // Cancel the SegmentCleanupTask before it runs if possible
+            if (segmentCleanupTaskFuture != null && !segmentCleanupTaskFuture.isCancelled()) {
+                segmentCleanupTaskFuture.cancel(false);
+            }
             lastOperation = operation;
         }
     }
 
     /**
      * If connect was the first method invoked from the HttpURLConnection APIs then a
-     * TimerTask will be started which will determine if the segment should be ignored or not.
+     * cleanup task will be started which will determine if the segment should be ignored or not.
      * <p>
      * Note: If the user configurable segment_timeout is explicitly configured to be lower than the timer delay set
      * here then the segment timing will already have been ended and trying to end/ignore it again will have no effect.
      */
-    private void startSegmentExpirationTimerTask() {
-        Timer timer = new Timer("HttpURLConnection Segment Expiration Timer");
-        TimerTask task = new TimerTask() {
-            public void run() {
-                ignoreSegmentForNonNetworkCall();
-            }
-        };
-
+    private void startSegmentCleanupTask() {
         /*
          * The following tests do a Thread.sleep to account for this delay. If this value is changed then the tests will also need to be updated.
          * functional_test/src/test/java/com/newrelic/agent/instrumentation/pointcuts/net/HttpURLConnectionTest
          * instrumentation/httpurlconnection/src/test/java/com/nr/agent/instrumentation/httpurlconnection/MetricStateConnectTest
          */
         long segmentExpirationDelayInMillis = 60_000L;
-        timer.schedule(task, segmentExpirationDelayInMillis);
+        // Submit a SegmentCleanupTask to a ScheduledThreadPoolExecutor to be run after a configured delay
+        SegmentCleanupTask segmentCleanupTask = new SegmentCleanupTask("New Relic HttpURLConnection Segment Cleanup Task");
+        segmentCleanupTaskFuture = threadPool.schedule(segmentCleanupTask, segmentExpirationDelayInMillis, TimeUnit.MILLISECONDS);
+    }
+
+    /**
+     * A Runnable task that can be scheduled to run to determine if a segment should be ignored or not
+     */
+    private class SegmentCleanupTask implements Runnable {
+        String taskName;
+
+        public SegmentCleanupTask(String taskName) {
+            this.taskName = taskName;
+        }
+
+        public void run() {
+            Thread.currentThread().setName(taskName);
+            ignoreSegmentForNonNetworkCall();
+        }
     }
 
     /**
-     * This method executes when a TimerTask completes. If it is determined that connect was the first and only HttpURLConnection API called
+     * This method executes when a cleanup task completes. If it is determined that connect was the first and only HttpURLConnection API called
      * then it will have the effect of ignoring the segment, as no external call has been made. A supportability metric will also be recorded.
      * The purpose of this is to avoid hitting the default segment timeout of 10 minutes and to also prevent the segment from showing in traces.
      */