Handling recycling race and volatile read-writes in Apache Async client (#662)

Author: eyalkoren

CHANGELOG.md

@@ -19,6 +19,7 @@
  * ClassCastException related to async instrumentation of Pilotfish Executor causing thread hang (applied workaround)
  * NullPointerException when computing Servlet transaction name with null HTTP method name
  * FileNotFoundException when trying to find implementation version of jar with encoded URL
+ * NullPointerException when closing Apache AsyncHttpClient request producer
 
 # 1.6.1
 

apm-agent-plugins/apm-apache-httpclient-plugin/src/main/java/co/elastic/apm/agent/httpclient/HttpAsyncRequestProducerWrapper.java

@@ -36,23 +36,21 @@
 import org.apache.http.nio.protocol.HttpAsyncRequestProducer;
 import org.apache.http.protocol.HttpContext;
 
-import javax.annotation.Nullable;
 import java.io.IOException;
 
 public class HttpAsyncRequestProducerWrapper implements HttpAsyncRequestProducer, Recyclable {
     private final ApacheHttpAsyncClientHelperImpl helper;
-    private HttpAsyncRequestProducer delegate;
-    private @Nullable HttpRequest request;
-    private volatile Span span;
+    private volatile HttpAsyncRequestProducer delegate;
+    private Span span;
 
     HttpAsyncRequestProducerWrapper(ApacheHttpAsyncClientHelperImpl helper) {
         this.helper = helper;
     }
 
     public HttpAsyncRequestProducerWrapper with(HttpAsyncRequestProducer delegate, Span span) {
-        // Order is important due to visibility - write to span last on this (initiating) thread
-        this.delegate = delegate;
+        // Order is important due to visibility - write to delegate last on this (initiating) thread
         this.span = span;
+        this.delegate = delegate;
         return this;
     }
 
@@ -63,27 +61,26 @@ public HttpHost getTarget() {
 
     @Override
     public HttpRequest generateRequest() throws IOException, HttpException {
-        // first read the volatile span
-        final Span localSpan = this.span;
-        request = delegate.generateRequest();
+        // first read the volatile, span will become visible as a result
+        HttpRequest request = delegate.generateRequest();
 
         if (request != null) {
             RequestLine requestLine = request.getRequestLine();
             if (requestLine != null) {
                 String method = requestLine.getMethod();
-                localSpan.withName(method).appendToName(" ");
+                span.withName(method).appendToName(" ");
                 span.getContext().getHttp().withMethod(method).withUrl(requestLine.getUri());
             }
 
-            request.setHeader(TraceContext.TRACE_PARENT_HEADER, localSpan.getTraceContext().getOutgoingTraceParentHeader().toString());
+            request.setHeader(TraceContext.TRACE_PARENT_HEADER, span.getTraceContext().getOutgoingTraceParentHeader().toString());
         }
 
         HttpHost host = getTarget();
         //noinspection ConstantConditions
         if (host != null) {
             String hostname = host.getHostName();
             if (hostname != null) {
-                localSpan.appendToName(hostname);
+                span.appendToName(hostname);
             }
         }
 
@@ -118,14 +115,14 @@ public void resetRequest() throws IOException {
 
     @Override
     public void close() throws IOException {
-        helper.recycle(this);
         delegate.close();
+        helper.recycle(this);
     }
 
     @Override
     public void resetState() {
-        request = null;
-        delegate = null;
+        // Order is important due to visibility - write to delegate last
         span = null;
+        delegate = null;
     }
 }