tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Filip Hanik - Dev Lists <devli...@hanik.com>
Subject Re: WebSocket status
Date Thu, 01 Mar 2012 22:55:51 GMT

hi Mark, sorry, I know you posted this earlier, thanks for reposting it to set me straight.
Yes, it can make sense that you see the behavior you are. But in "theory" you shouldn't.

If you call readSocket(false,bytes,off,maxRead), all the socket does is that it checks in
the OS socket buffer to see if there is any data 
available. If not, it immediately returns 0. This is the non blocking part of it. Now, since
you call this part of a servlet, the socket is 
set in zero interest mode (meaning, we don't want the socket poller to be reacting if data
is coming in, this will cause concurrency issues 
and poor performance)

So, what happens is that

1. You call read(block=false)
2. Your 1) call finishes, returns 0
3. Data arrives, ends up in the OS TCP receive buffer
4. You finish your "servlet request lifecycle" and return the socket to the poller, with read
interest


In step four, the poller should awake immediately for a read operation if there is data in
the.

However, what seems to be happening is a misuse of NIO in Http11NioProtocol
         @Override
         protected void upgradePoll(SocketWrapper<NioChannel> socket,
                 Processor<NioChannel> processor) {
             connections.put(socket.getSocket(), processor);

             SelectionKey key = socket.getSocket().getIOChannel().keyFor(
                     socket.getSocket().getPoller().getSelector());
             key.interestOps(SelectionKey.OP_READ);
             ((KeyAttachment) socket).interestOps(
                     SelectionKey.OP_READ);
         }

You can't register on a selector using another thread. This is most likely the cause of the
problem, is the incorrect registration. You see, 
you shouldn't be touching the poller from anywhere in the code itself. When the SocketProcessor
returns, it needs to decide if the socket 
goes back into the poller, for what operation, or if it needs to be cancelled/closed.

If you look at SocketProcessor.run method, the very last thing that is happening here is
                         final SelectionKey fk = key;
                         final int intops = handshake;
                         final KeyAttachment ka = (KeyAttachment)fk.attachment();
                         ka.getPoller().add(socket,intops);

But my guess is that these new if/else clauses are bypassing this.

I'll work on a fix for this, and check it in so that you can see.

On a completely separate note, this WebSocket implementation seems to have caused a lot of
duplicate code. I would have assumed the most 
simple way to implement WebSockets would be on top of the existing Comet implementation. This
implementation already generates the READ 
event when data arrives, and you have the ability to check available() to see if there is
more data to be read. This would too have avoided 
single byte reads from the system buffers, since Comet reads in as much as possible, and then
you can single byte read from Java heap memory.

The only thing you would have had to do, had you used Comet, was to turn off the filters that
cause chunked-encoding, and you would have had 
access to raw data directly from within. All the Upgrade classes, everything, would have sat
directly in the highest level, never touching 
the low level connectors.

Something food for thought.... :)



On 3/1/2012 11:15 AM, Mark Thomas wrote:
> On 01/03/2012 18:01, Filip Hanik - Dev Lists wrote:
>> Alright, now that I'm all squared away with Autobahn and test cases
>> running. Is there a specific unit test you have to produce this behavior?
>> That would help me for digging through it.
> Assuming you are running trunk, then what you'll currently have is NIO
> is a purely blocking mode. The tests should all pass and take a minute
> or two to run. If you apply the following patch, you'll enable
> non-blocking reads for NIO at which point large numbers of tests should
> start to fail / take forever / timeout. I usually just run the section 1
> tests (i.e. "cases": ["1.*"]) in fuzzing_client_spec.json when
> investigating these are they are mostly small payloads easy to trace.
>
> HTH,
>
> Mark
>
> --- a/java/org/apache/coyote/http11/upgrade/UpgradeNioProcessor.java
> +++ b/java/org/apache/coyote/http11/upgrade/UpgradeNioProcessor.java
> @@ -104,12 +104,10 @@ public class UpgradeNioProcessor extends
> UpgradeProcessor<NioChannel>  {
>       @Override
>       public int read(boolean block, byte[] bytes, int off, int len)
>               throws IOException {
> -        // TODO Implement non-blocking reads. Should be as simple as
> replacing
> -        // true with block in the two lines below
>           if (len>  maxRead) {
> -            return readSocket(true, bytes, off, maxRead);
> +            return readSocket(block, bytes, off, maxRead);
>           } else {
> -            return readSocket(true, bytes, off, len);
> +            return readSocket(block, bytes, off, len);
>           }
>       }
>
>> Filip
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message