Hi,
I wrote a unit test case.
Actually I just modified ProtocolParserTest because I think that it is enough in order to reproduce this issue.
I attached the diffs of the unit test and the proposed patch.
Here is the diff
----
Index: test/java/com/sun/grizzly/ProtocolParserTest.java
===================================================================
--- test/java/com/sun/grizzly/ProtocolParserTest.java (revision 3193)
+++ test/java/com/sun/grizzly/ProtocolParserTest.java (working copy)
@@ -44,6 +44,7 @@
import com.sun.grizzly.utils.ControllerUtils;
import com.sun.grizzly.utils.NonBlockingTCPIOClient;
import com.sun.grizzly.utils.NonBlockingSSLIOClient;
+import com.sun.grizzly.connectioncache.server.CacheableSelectionKeyHandler;
import java.io.File;
import java.io.IOException;
@@ -109,6 +110,33 @@
}
}
+ public void testSimplePacketUsingServerCache() throws IOException {
+ Controller controller = createController(PORT, new CacheableSelectionKeyHandler( 10, 1 ) );
+ NonBlockingTCPIOClient client = new NonBlockingTCPIOClient("localhost", PORT);
+
+ try {
+ byte[] testData = "Hello".getBytes();
+ ControllerUtils.startController(controller);
+ client.connect();
+ client.send(testData);
+
+ System.out.println("Sleeping 5 seconds");
+ try{
+ Thread.sleep(5 * 1000); //Wait 5 second before sending the bytes
+ } catch (Throwable t){
+
+ }
+ client.send(" Partial".getBytes());
+
+ byte[] response = new byte["Hello Partial".length()];
+ client.receive(response);
+ assertTrue(Arrays.equals("Hello Partial".getBytes(), response));
+ } finally {
+ controller.stop();
+ client.close();
+ }
+ }
+
public void testSimpleSSLPacket() throws IOException {
Controller controller = createSSLController(PORT);
NonBlockingSSLIOClient client = new NonBlockingSSLIOClient("localhost", PORT,sslConfig);
@@ -146,12 +174,18 @@
}
}
}
-
+
private Controller createController(int port) {
+ return createController( port, null );
+ }
+
+ private Controller createController(int port, SelectionKeyHandler selectionKeyHandler ) {
final ProtocolFilter echoFilter = new EchoFilter();
final ProtocolFilter parserProtocolFilter = createParserProtocolFilter();
TCPSelectorHandler selectorHandler = new TCPSelectorHandler();
selectorHandler.setPort(port);
+ if( selectionKeyHandler != null )
+ selectorHandler.setSelectionKeyHandler( selectionKeyHandler );
final Controller controller = new Controller();
Index: main/java/com/sun/grizzly/connectioncache/server/CacheableSelectionKeyHandler.java
===================================================================
--- main/java/com/sun/grizzly/connectioncache/server/CacheableSelectionKeyHandler.java (revision 3193)
+++ main/java/com/sun/grizzly/connectioncache/server/CacheableSelectionKeyHandler.java (working copy)
@@ -85,6 +85,7 @@
@Override
public void postProcess(SelectionKey key) {
+ super.postProcess( key );
SelectableChannel channel = key.channel();
inboundConnectionCache.requestProcessed(channel, 1);
inboundConnectionCache.responseSent(channel);
----
I think that you can reproduce this issue with modified ProtocolParserTest easily.
And if this issue will be comfirmed, I will file this issue.
Thanks.
--
Bongjae Chang
----- Original Message -----
From: "Bongjae Chang" <carryel_at_korea.com>
To: <dev_at_grizzly.dev.java.net>
Sent: Friday, May 15, 2009 10:34 AM
Subject: Re: A CacheableSelectionKeyHandler's bug and a question about WorkerThread.detach()
>
> Hi,
>
> Jeanfrancois Arcand wrote:
>> Hum...do you think you can write a unit test case that demonstrate that
>> issue? That will be quite usefull to add to the builld (and the fix :-))
>
> At first, I thought that you just had omitted the line "super.postProcess( key )" in CacheableSelectionKeyHandler#postProcess() by mistake.
>
> But maybe it seems that you have purposed it when I read your mail.
>
> Then, I think that a unit test case and more logs which demonstrate my words needs, too.
>
> I will try to write that and reply again.
>
> Thanks.
> --
> Bongjae Chang
>
>
> ----- Original Message -----
> From: "Jeanfrancois Arcand" <Jeanfrancois.Arcand_at_Sun.COM>
> To: <dev_at_grizzly.dev.java.net>
> Sent: Thursday, May 14, 2009 11:51 PM
> Subject: Re: A CacheableSelectionKeyHandler's bug and a question about WorkerThread.detach()
>
>
>> Salut,
>>
>> Bongjae Chang wrote:
>>> I would like to illustrate some more in detail.
>>>
>>> (1)
>>> /Bongjae wrote:/
>>> />I know that ParserProtocolFilter saves the current thread's ByteBuffer
>>> and attaches the ThreadAttachment to SelectionKey if
>>> ProtocolParser's isExpectingMoreData() returns true. If detach() is not
>>> executed at this case, *the remaining ByteBuffer can cause a parsing
>>> error*./
>>>
>>> If BaseSelectionKeyHandler#postProcess() is not called like current
>>> CacheableSelectionKeyHandler#postProcess(), in other words
>>> WokerThread#detach() is not called,
>>>
>>> Current thread's byteBuffer(A) which have been stored in
>>> ThreadAttachment(B) will be clear() by WorkerThread#reset() because
>>> DefaultThreadPool#afterExecute() will be executed after executing the task.
>>>
>>> Then, the byteBuffer(A) will be shared by ThreadAttachmemt(B)'s
>>> byteBuffer because it is the same reference. So, the next step for
>>> parsing the incomplete message will use the missing data of the buffer
>>> because ByteBuffer's position already has been reset.
>>
>> Hum...do you think you can write a unit test case that demonstrate that
>> issue? That will be quite usefull to add to the builld (and the fix :-))
>>
>>
>>>
>>>
>>> (2)
>>> /Bongjae wrote:/
>>> />When I reviewed the WorkerThread's detach() method, a question arised.
>>> Why does WorkerThread create new ByteBuffer in BYTE_BUFFER mode? *The
>>> new ByteBuffer could become to be meaningless* if the SelectionKey had a
>>> saved ByteBuffer, doesn't it?/
>>>
>>> If it is, I think that "byteBuffer = null" is better than
>>> "createByteBuffer(true)" in point of being able to cut down on the
>>> overhead of redundant ByteBuffer's allocation.
>>
>> Agree. But we must make sure next time the WorkerThread is used, the
>> ByteBuffer gets properly created.
>>
>> Thanks!
>>
>> -- Jeanfrancois
>>
>>>
>>> Thanks.
>>>
>>> --
>>> Bongjae Chang
>>>
>>>
>>>
>>> ----- Original Message -----
>>> *From:* Bongjae Chang <mailto:carryel_at_korea.com>
>>> *To:* dev_at_grizzly.dev.java.net <mailto:dev_at_grizzly.dev.java.net>
>>> *Sent:* Wednesday, May 13, 2009 10:07 PM
>>> *Subject:* A CacheableSelectionKeyHandler's bug and a question about
>>> WorkerThread.detach()
>>>
>>> Hi,
>>>
>>> I tried to use CacheableSelectionKeyHandler, but some problems were
>>> occurred when ProtocolParser's isExpectingMoreData() returned true.
>>>
>>> In conclusion, CacheableSelectionKeyHandler#postProcess(SelectionKey
>>> key) didn't call super.postProcess( key ).
>>>
>>> In CacheableSelectionKeyHandler.java
>>> ----
>>> public void postProcess(SelectioKey key) {
>>> * super.postProcess( key );*
>>> ...
>>> }
>>> ----
>>>
>>> So WokerThread's detach() couldn't be called and the byteBuffer was
>>> recycled inadequately.
>>>
>>> I know that ParserProtocolFilter saves the current thread's
>>> ByteBuffer and attaches the ThreadAttachment to SelectionKey if
>>> ProtocolParser's isExpectingMoreData() returns true. If detach() is
>>> not executed at this case, the remaining ByteBuffer can cause a
>>> parsing error.
>>>
>>> When I reviewed the WorkerThread's detach() method, a question
>>> arised. Why does WorkerThread create new ByteBuffer in BYTE_BUFFER
>>> mode? The new ByteBuffer could become to be meaningless if the
>>> SelectionKey had a saved ByteBuffer, doesn't it?
>>>
>>> In WorkerThreadImpl.java
>>> ----
>>> public ThreadAttachment detach() {
>>> ...
>>> if((mode & Mode.BYTE_BUFFER) != 0 ) {
>>> createByteBuffer(true);
>>> }
>>> }
>>> ----
>>>
>>> And DefaultThreadPool already creates a ByteBuffer when it is equal
>>> to null in beforeExecute().
>>>
>>> In DefaultThreadPool.java
>>> ----
>>> protected void beforeExecute(Thread t, Runnable r ) {
>>> ((WorkerThreadImpl) t).createByteBuffer(false);
>>> }
>>> ----
>>>
>>> Perhaps, is it for the purpose of eliminating the dependency of the
>>> thread pool's impl?
>>>
>>> If detach() is modified like this,
>>> ----
>>> public ThreadAttachment detach() {
>>> ...
>>> if((mode & Mode.BYTE_BUFFER) != 0 ) {
>>> byteBuffer = null;
>>> //createByteBuffer(true);
>>> }
>>> }
>>> ----
>>>
>>> Could any side-effect be occurred?
>>>
>>> Please advice me.
>>>
>>> Thanks.
>>>
>>> --
>>> Bongjae Chang
>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe_at_grizzly.dev.java.net
>> For additional commands, e-mail: dev-help_at_grizzly.dev.java.net
>>
>>
>>
>>