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

ProtocolEncoderOutputImpl.flush() throws an IllegalArgumentException if buffers queue is empty

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.7
    • Fix Version/s: 2.0.8
    • Component/s: None
    • Labels:
      None

      Description

      https://git-wip-us.apache.org/repos/asf?p=mina.git;a=blob;f=mina-core/src/main/java/org/apache/mina/filter/codec/ProtocolCodecFilter.java;h=f413491e058ff1c5a24c3b2ccae6d736f481bbaf;hb=2.0#l428

      First, flush method initializes WriteFuture with a null reference.
      The object can be constructed later, if buffers queue isn't empty, but otherwise remains untouched.
      If it still holds a null reference, the DefaultWriteRequest constructor call always fails, as it doesn't accept null as a message param.

      Excerpt from the stack trace:

      Caused by: java.lang.IllegalArgumentException: message
      at org.apache.mina.core.write.DefaultWriteRequest.<init>(DefaultWriteRequest.java:133)
      at org.apache.mina.filter.codec.ProtocolCodecFilter$ProtocolEncoderOutputImpl.flush(ProtocolCodecFilter.java:448)
      at com.ugcs.messaging.mina.MinaEncoder.encode(MinaEncoder.java:27)
      at org.apache.mina.filter.codec.ProtocolCodecFilter.filterWrite(ProtocolCodecFilter.java:308)
      ... 33 more
      

        Issue Links

          Activity

          Hide
          elecharny Emmanuel Lecharny added a comment -

          I'm not sure what you are suggesting here...

          Do you think that we should always send back a Future instead of null, even if the queue is empty ?

          Now, regarding the Stack trace you get, you clearly have a race condition occuring. In the flush method, we do :

                      while (!bufferQueue.isEmpty()) {
                          Object encodedMessage = bufferQueue.poll();
          
                          if (encodedMessage == null) {
                              break;
                          }
          
                          nextFilter.filterWrite(session, new EncodedWriteRequest(encodedMessage, future, destination));
                      }
          

          (I removed the irrelevant parts)

          The new EncodedWriteRequest() part will call the super constructor, which does :

              public DefaultWriteRequest(Object message, WriteFuture future, SocketAddress destination) {
                  if (message == null) {
                      throw new IllegalArgumentException("message");
                  }
          

          At this point, the message should never be null. If it's null, that means the encodedMessage has been nullified between the if (encodedMessage == null ) test and the construction of the EncodedWriteRequest...

          Show
          elecharny Emmanuel Lecharny added a comment - I'm not sure what you are suggesting here... Do you think that we should always send back a Future instead of null, even if the queue is empty ? Now, regarding the Stack trace you get, you clearly have a race condition occuring. In the flush method, we do : while (!bufferQueue.isEmpty()) { Object encodedMessage = bufferQueue.poll(); if (encodedMessage == null ) { break ; } nextFilter.filterWrite(session, new EncodedWriteRequest(encodedMessage, future , destination)); } (I removed the irrelevant parts) The new EncodedWriteRequest() part will call the super constructor, which does : public DefaultWriteRequest( Object message, WriteFuture future , SocketAddress destination) { if (message == null ) { throw new IllegalArgumentException( "message" ); } At this point, the message should never be null. If it's null, that means the encodedMessage has been nullified between the if (encodedMessage == null ) test and the construction of the EncodedWriteRequest...
          Hide
          jenya Jenya Pisarenko added a comment -

          I'm looking at the 2.0 source. The ProtocolEncoderOutputImpl.flush() here looks as follows:

                  public WriteFuture flush() {
                      Queue<Object> bufferQueue = getMessageQueue();
                      WriteFuture future = null;
          
                      while (!bufferQueue.isEmpty()) {
                          Object encodedMessage = bufferQueue.poll();
          
                          if (encodedMessage == null) {
                              break;
                          }
          
                          // Flush only when the buffer has remaining.
                          if (!(encodedMessage instanceof IoBuffer) || ((IoBuffer) encodedMessage).hasRemaining()) {
                              future = new DefaultWriteFuture(session);
                              nextFilter.filterWrite(session, new EncodedWriteRequest(encodedMessage, future, destination));
                          }
                      }
          
                      if (future == null) {
                          // Creates an empty writeRequest containing the destination
                          WriteRequest writeRequest = new DefaultWriteRequest(null, null, destination);
                          future = DefaultWriteFuture.newNotWrittenFuture(session, new NothingWrittenException(writeRequest));
                      }
          
                      return future;
                  }
          

          Queue iteration part is ok. As future variable is assigned only in a loop, The last if consequent is invoked, when the loop condition is never true within the method, i.e. bufferQueue.isEmpty() == true by the flush() call. It should be possible to achieve in two sequential flush() calls without any concurrency (assuming the getMessageQueue() can return an empty collection).

          The first line of the if block:

                          // Creates an empty writeRequest containing the destination
                          WriteRequest writeRequest = new DefaultWriteRequest(null, null, destination);
          

          always fails, because of the IllegalArgumentException, thus the rest of it:

                          future = DefaultWriteFuture.newNotWrittenFuture(session, new NothingWrittenException(writeRequest));
          

          is never executed.

          I suggest to fix the last part of the method so it could correctly create an empty write request/write future.

          Show
          jenya Jenya Pisarenko added a comment - I'm looking at the 2.0 source. The ProtocolEncoderOutputImpl.flush() here looks as follows: public WriteFuture flush() { Queue< Object > bufferQueue = getMessageQueue(); WriteFuture future = null ; while (!bufferQueue.isEmpty()) { Object encodedMessage = bufferQueue.poll(); if (encodedMessage == null ) { break ; } // Flush only when the buffer has remaining. if (!(encodedMessage instanceof IoBuffer) || ((IoBuffer) encodedMessage).hasRemaining()) { future = new DefaultWriteFuture(session); nextFilter.filterWrite(session, new EncodedWriteRequest(encodedMessage, future , destination)); } } if ( future == null ) { // Creates an empty writeRequest containing the destination WriteRequest writeRequest = new DefaultWriteRequest( null , null , destination); future = DefaultWriteFuture.newNotWrittenFuture(session, new NothingWrittenException(writeRequest)); } return future ; } Queue iteration part is ok. As future variable is assigned only in a loop, The last if consequent is invoked, when the loop condition is never true within the method, i.e. bufferQueue.isEmpty() == true by the flush() call. It should be possible to achieve in two sequential flush() calls without any concurrency (assuming the getMessageQueue() can return an empty collection). The first line of the if block: // Creates an empty writeRequest containing the destination WriteRequest writeRequest = new DefaultWriteRequest( null , null , destination); always fails, because of the IllegalArgumentException , thus the rest of it: future = DefaultWriteFuture.newNotWrittenFuture(session, new NothingWrittenException(writeRequest)); is never executed. I suggest to fix the last part of the method so it could correctly create an empty write request/write future.
          Hide
          elecharny Emmanuel Lecharny added a comment -

          Got it now !!!

          Good catch. We need to inject a empty object (it can be an instance of a class like EmptyMessage, a class we can create for this purpose).

          Let me try that.

          Show
          elecharny Emmanuel Lecharny added a comment - Got it now !!! Good catch. We need to inject a empty object (it can be an instance of a class like EmptyMessage, a class we can create for this purpose). Let me try that.
          Hide
          elecharny Emmanuel Lecharny added a comment -

          Should be fixed with Commit: 3b793f343263a6750fd034b725db83e89bc345c3

          Show
          elecharny Emmanuel Lecharny added a comment - Should be fixed with Commit: 3b793f343263a6750fd034b725db83e89bc345c3

            People

            • Assignee:
              Unassigned
              Reporter:
              jenya Jenya Pisarenko
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development