Uploaded image for project: 'MINA'
  1. MINA
  2. DIRMINA-1047

NullPointerException in AbstractIoSession.destroy()

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.15
    • Fix Version/s: 2.0.16
    • Component/s: Core
    • Labels:
      None
    • Environment:
      Any

      Description

      After upgrading to v2.0.15, we get this NPE ...

      java.lang.NullPointerException
      	at org.apache.mina.core.session.AbstractIoSession.destroy(AbstractIoSession.java:369)
      	at org.apache.mina.core.session.AbstractIoSession.closeNow(AbstractIoSession.java:350)
      	at org.apache.mina.core.session.DefaultIoSessionDataStructureFactory$DefaultWriteRequestQueue.poll(DefaultIoSessionDataStructureFactory.java:222)
      	at org.apache.mina.core.polling.AbstractPollingIoProcessor.flushNow(AbstractPollingIoProcessor.java:827)
      	at org.apache.mina.core.polling.AbstractPollingIoProcessor.flush(AbstractPollingIoProcessor.java:767)
      	at org.apache.mina.core.polling.AbstractPollingIoProcessor.access$700(AbstractPollingIoProcessor.java:68)
      	at org.apache.mina.core.polling.AbstractPollingIoProcessor$Processor.run(AbstractPollingIoProcessor.java:1125)
      	at org.apache.mina.util.NamePreservingRunnable.run(NamePreservingRunnable.java:64)
      	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
      	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
      	at java.lang.Thread.run(Thread.java:745)
      

      If I'm reading it right, the destroy also needs to allow that the writeRequest returned by the poll() of the WriteRequestQueue may return null if the queue is empty.

      In our case the destroy was invoked after polling a CLOSE_REQUEST caused by an earlier closeOnFlush(), and the write request queue was empty.

      AbstractIoSession.java
          protected void destroy() {
              if (writeRequestQueue != null) {
                  while (!writeRequestQueue.isEmpty(this)) {
                      WriteRequest writeRequest = writeRequestQueue.poll(this);
                      WriteFuture writeFuture = writeRequest.getFuture();
                      
                      // The WriteRequest may not always have a future : The CLOSE_REQUEST
                      // and MESSAGE_SENT_REQUEST don't.
                      if (writeFuture != null) {
                          writeFuture.setWritten();
                      }
                  }
              }
          }
      

        Issue Links

          Activity

          Hide
          cwicklow Carl Wicklow added a comment - - edited

          To be clear : we are using 2.0.15 (Jira doesn't let me choose that as the affected versions, although 2.0.14 would also throw this NPE).

          For what it's worth, this is very similar to DIRMINA-1043.

          That fix almost resolved this too, but It looks like it needs one more null check to be solid.

          Show
          cwicklow Carl Wicklow added a comment - - edited To be clear : we are using 2.0.15 (Jira doesn't let me choose that as the affected versions, although 2.0.14 would also throw this NPE). For what it's worth, this is very similar to DIRMINA-1043 . That fix almost resolved this too, but It looks like it needs one more null check to be solid.
          Hide
          elecharny Emmanuel Lecharny added a comment -

          I have fixed the release to 2.0.15 (I forgot to close the release in JIRA).

          Looking at the problem. Thanks for the report !

          Show
          elecharny Emmanuel Lecharny added a comment - I have fixed the release to 2.0.15 (I forgot to close the release in JIRA). Looking at the problem. Thanks for the report !
          Hide
          elecharny Emmanuel Lecharny added a comment -

          yeah, seems like we have a race condition here : the writeQueue is not empty when we test it, but it's emptied when we poll from it.

          Something like that should fix the NPE :

              protected void destroy() {
                  if (writeRequestQueue != null) {
                      while (!writeRequestQueue.isEmpty(this)) {
                          WriteRequest writeRequest = writeRequestQueue.poll(this);
                          
                          if (writeRequest != null) {
                              WriteFuture writeFuture = writeRequest.getFuture();
                              
                              // The WriteRequest may not always have a future : The CLOSE_REQUEST
                              // and MESSAGE_SENT_REQUEST don't.
                              if (writeFuture != null) {
                                  writeFuture.setWritten();
                              }
                          }
                      }
                  }
              }
          

          Can you test it ? (it builds with no error on my machine)

          Show
          elecharny Emmanuel Lecharny added a comment - yeah, seems like we have a race condition here : the writeQueue is not empty when we test it, but it's emptied when we poll from it. Something like that should fix the NPE : protected void destroy() { if (writeRequestQueue != null) { while (!writeRequestQueue.isEmpty(this)) { WriteRequest writeRequest = writeRequestQueue.poll(this); if (writeRequest != null) { WriteFuture writeFuture = writeRequest.getFuture(); // The WriteRequest may not always have a future : The CLOSE_REQUEST // and MESSAGE_SENT_REQUEST don't. if (writeFuture != null) { writeFuture.setWritten(); } } } } } Can you test it ? (it builds with no error on my machine)
          Hide
          cwicklow Carl Wicklow added a comment -

          I can test it (I may not get that done today, but sometime soon).

          Show
          cwicklow Carl Wicklow added a comment - I can test it (I may not get that done today, but sometime soon).
          Hide
          elecharny Emmanuel Lecharny added a comment -

          I have committed the fix, FTR.

          Show
          elecharny Emmanuel Lecharny added a comment - I have committed the fix, FTR.
          Hide
          cwicklow Carl Wicklow added a comment -

          Just to confirm, that fix works for me.

          Show
          cwicklow Carl Wicklow added a comment - Just to confirm, that fix works for me.
          Hide
          elecharny Emmanuel Lecharny added a comment -

          Marked as fixed.

          Show
          elecharny Emmanuel Lecharny added a comment - Marked as fixed.

            People

            • Assignee:
              Unassigned
              Reporter:
              cwicklow Carl Wicklow
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development