eclipse-ee4j / eclipse-ee4j/jersey
JettyConnector race-condition causes connection exception not to be thrown but instead returns flawed Response object (causing response.getStatus() to NPE)
- Dominant language
- Java
- Stars
- 730
- Forks
- 382
- PR merge metrics
- No merged PRs in 30d
Description
Using latest jersey 2.30.1 (jersey-bom) and jetty 9.4.28.v20200408
and jersey-jetty-connector
I ran into a situation where I perform a request async() (which returns a requestFuture) - the back-end server is not running so a connection exception is expected.
Then requestFuture.get() sometimes returns without throwing the expected connection exception --> sometimes it does and sometimes it doesn't so we're definitely talking about a 'race condition' (probably testing this on my laptop with 6 cores / 12 virtual cores and some additional load in the webbrowser affect the success rate on naturally replicating the problem but below I explain how the situation can be simulated in the debugger by setting strategic breakpoints to play each thread at desired speed as it happens during such race condition; this way the problem is 100% reproduceable)
Instead requestFuture.get() returns a flawed Response object with null context.
When you perform response.getStatus() you receive an NPE:
==> java.lang.NullPointerException
at: InboundJaxrsResponse.getStatus(InboundJaxrsResponse.java:73)
Not getting the connection exception and receiving a flawed Response object which causes an NPE on getStatus() seemed really odd so I've been debugging the past 24hr to get to the bottom of this!
I can just show an excerpt of the code and have not found the time to build a smaller ready to run test harness yet but that should be fairly easy from this:
```
String requestEntity = "[ 1, 2, 3 ]"
final String contentType = "application/json";
WebTarget target = client.target("http://localhost:8080/test");
Builder builder = target.request();
Response response;
Future responseFuture;
try {
responseFuture = builder.async().method(requestMethodUppercase,
Entity.entity(requestEntity, MediaType.valueOf(contentType)))
response = responseFuture.get();
} catch(Exception e) {
LOG.error("request failed", e); // <--- no exception caught although connection refused!
}
LOG.info("Request status={}", response.getStatus()); // <--- NPE here!
```
Notice that the reason we started to use async() was to avoid the hardcoded 2MB POST entity size limitation as set in the default constructor of jetty BufferingResponseListener() [ at least that's what I recall at this moment, nevertheless async mode should work trouble free..]
When you run this code outside of a debugger, and assuming there's no server listening on localhost port 8080, you will and should normally catch the ExecutionException/ wrapped IOException:
>==> java.util.concurrent.ExecutionException - javax.ws.rs.ProcessingException: java.io.IOException: >java.net.ConnectException: Connection refused: no further information
> at: CompletableFuture.reportGet(CompletableFuture.java:357)
> - Cause: javax.ws.rs.ProcessingException - java.io.IOException: java.net.ConnectException: >Connection refused: no further information
> at: ClientRuntime.processFailure(ClientRuntime.java:246)
> - Cause: java.io.IOException - java.net.ConnectException: Connection refused: no further >information
> at: DeferredContentProvider.flush(DeferredContentProvider.java:197)
> - Cause: java.net.ConnectException - Connection refused: no further information
> at: SocketChannelImpl.checkConnect(Native Method)
However I found a particular race condition - caused by the code in JettyConnector as I understand the problem now.
You can re-enact the thread race condition by setting strategic breakpoints in your IDE (only break at thread level):
- JettyConnector:460 `callback.failure(failure);`
- JettyConnector: 448 ` if (callbackInvoked.compareAndSet(false, true)) {`
Execute the above code to perform the request and wait till the debugger stops in jersey-client-async-executor thread at JettyConnector:448. Do not let the callback.failure(failure) method execute.
Thread information here is:
> "jersey-client-async-executor-0@6278" prio=5 tid=0x66 nid=NA runnable
> java.lang.Thread.State: RUNNABLE
> at org.glassfish.jersey.jetty.connector.JettyConnector.apply(JettyConnector.java:460)
Notice that `callbackInvoked.compareAndSet(false, true)` has now been executed so callbackInvoked is now 'true' (but by suspending the thread the callback has not yet really been invoked!! Exactly the way to show the race condition).
Now in thread HttpClient is waiting at JettyConnector:448 :
> "HttpClient@3a5c2626-96@6224" prio=5 tid=0x60 nid=NA runnable
> java.lang.Thread.State: RUNNABLE
> at org.glassfish.jersey.jetty.connector.JettyConnector$3.onFailure(JettyConnector.java:448)
>
```
if (callbackInvoked.compareAndSet(false, true)) {
callback.failure(t);
}
```
If you proceed this thread now it'll skip invoking the callback as callbackInvoked has already been set to true by the jersey-client-async-executor thread (although the callback has just not yet been invoked on that thread).
After proceeding the onComplete handler will be invoked from the HttpClient thread:
> JettyConnector:436..441
```
@Override
public void onComplete(final Result result) {
entityStream.closeQueue();
callback.response(jerseyResponse.get());
responseFuture.complete(jerseyResponse.get());
}
```
In the onComplete handler there is no check for callbackInvoked at all!
So callback.response(jerseyResponse.get()) will get invoked and send a null response value into the callback!
This creates a completely flawed Response object with null context and wakes-up the async() future in the main method - returns this flawed response to my code and response.getStatus() will NPE without the connection failed exception ever been detected!!
Contributor guide
Assessment
This issue has not been assessed yet.