[HTTPCORE-659] Race condition in IOSessionImpl when setting event

• Type: Bug
• Status: Resolved
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 5.0.3
• Fix Version/s: 5.1-beta3, 5.1
• Component/s: HttpCore NIO
• Assignee: Unassigned
• Reporter: Nicolas Capponi
• Created: 19/Jan/21 17:12
• Updated: 02/Apr/21 10:46
• Resolved: 26/Jan/21 08:10

Description

There is a race condition in class org.apache.hc.core5.reactor.IOSessionImpl when setting an event through the setEvent(final int op) method: there is a check on the session to ensure it is not closed but this check is performed outside the lock that is later acquired to protect the call on the SelectionKey. If the session is closed just after the check but before getting the lock, then it will throw a CancelledKeyException when trying to perform the call on the SelectionKey.

I have implemented an HTTP server that use the reactive library (httpcore5-reactive component) and execute the requests in a separate thread pool. I was hit by this issue, here is a stack trace (with traces non specific to HttpCore library removed):

   CancelledKeyException at SelectionKeyImpl.java:71 
   SelectionKeyImpl.java:90 
   IOSessionImpl.java:159 
   InternalDataChannel.java:324 
   AbstractHttp1StreamDuplexer.java:  452 
   ServerHttp1StreamDuplexer.java:136 
   ServerHttp1StreamHandler.java:94 
   ReactiveDataProducer.java:108 
   ReactiveDataProducer.java:100 
   ... traces from publisher implementation using Rx java library... 
   ReactiveServerExchangeHandler.java:91          
   ReactiveServerExchangeHandler.java:83 
   ...

 

I was able to reproduce it almost systematically with unit test from my application (concurrency issues are very sensitive to time). If necessary I can provide a sample HttpAsyncServer which allows me to reproduce it by introducing a simple Thread.sleep(50) sequence in IOSessionImpl code.

There is a simple fix: move the check on close status inside the lock-unlock block

   lock.lock();
   try {
       if (isStatusClosed()) {
            return;
       }
       this.key.interestOps(this.key.interestOps() | op);
   } finally {
     lock.unlock();
   }

Note that 2 other methods of this class are subject to this problem: setEventMask(final int newValue) and clearEvent(final int op). The fix is the same than for the first method.

I can submit a pull request with the fix(es) if you indicate me in which branch I should submit it.