Salut,
Bongjae Chang wrote:
> 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.
I was able to reproduce the issue. Go ahead and file an issue with the diff.
Thanks!
-- Jeanfrancois
>
> 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
>>>
>>>
>>>
>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe_at_grizzly.dev.java.net
>>> For additional commands, e-mail: dev-help_at_grizzly.dev.java.net