You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@mina.apache.org by James Talmage <ja...@jrtechnical.com> on 2009/03/13 05:17:37 UTC
Potential DemuxingProtocolDecoderBug
See test case for explanation
///TEST CASE BELOW
import junit.framework.TestCase;
import org.apache.mina.filter.codec.demux.MessageDecoderResult;
import org.apache.mina.filter.codec.demux.MessageDecoderAdapter;
import org.apache.mina.filter.codec.demux.DemuxingProtocolDecoder;
import org.apache.mina.filter.codec.ProtocolDecoderOutput;
import org.apache.mina.core.session.IoSession;
import org.apache.mina.core.session.DummySession;
import org.apache.mina.core.buffer.IoBuffer;
import org.apache.mina.core.service.DefaultTransportMetadata;
import org.apache.mina.core.file.FileRegion;
import org.apache.mina.transport.socket.SocketSessionConfig;
import org.easymock.EasyMock;
import java.net.InetSocketAddress;
import java.nio.charset.Charset;
/**
* Simple Unit Test showing that the DemuxingProtocolDecoder has
* inconsistent behavior if used with a non fragmented transport.
*
*/
public class DemuxingProtocolDecoderBugTest extends TestCase
{
// PASSES
public void testFragmentedTransport() throws Exception
{
doTest(new SessionStub(true));
}
// FAILS
public void testNonFragmentedTransport() throws Exception
{
doTest(new SessionStub(false));
}
/*
testNonFragmentedTransport() fails with the following (truncated)
stack trace:
java.lang.AssertionError:
Expectation failure on verify:
write('B'): expected: 1, actual: 0
write(1): expected: 1, actual: 0
write(2): expected: 1, actual: 0
write('C'): expected: 1, actual: 0
.....
*/
public void doTest(IoSession session) throws Exception
{
ProtocolDecoderOutput mock =
EasyMock.createMock(ProtocolDecoderOutput.class);
mock.write(Character.valueOf('A'));
mock.write(Character.valueOf('B'));
mock.write(Integer.valueOf(1));
mock.write(Integer.valueOf(2));
mock.write(Character.valueOf('C'));
EasyMock.replay(mock);
IoBuffer buffer = IoBuffer.allocate(1000);
buffer.putString("AB12C", Charset.defaultCharset().newEncoder());
buffer.flip();
DemuxingProtocolDecoder decoder = new DemuxingProtocolDecoder();
decoder.addMessageDecoder(CharacterMessageDecoder.class);
decoder.addMessageDecoder(IntegerMessageDecoder.class);
decoder.decode(session,buffer,mock);
EasyMock.verify(mock);
}
public static class CharacterMessageDecoder extends
MessageDecoderAdapter
{
public MessageDecoderResult decodable(IoSession session, IoBuffer
in)
{
return Character.isDigit((char)in.get())
? MessageDecoderResult.NOT_OK
: MessageDecoderResult.OK;
}
public MessageDecoderResult decode(IoSession session, IoBuffer in,
ProtocolDecoderOutput out) throws Exception
{
out.write(Character.valueOf((char)in.get()));
return MessageDecoderResult.OK;
}
}
public static class IntegerMessageDecoder extends MessageDecoderAdapter
{
public MessageDecoderResult decodable(IoSession session, IoBuffer
in)
{
return Character.isDigit((char)in.get())
? MessageDecoderResult.OK
: MessageDecoderResult.NOT_OK;
}
public MessageDecoderResult decode(IoSession session, IoBuffer in,
ProtocolDecoderOutput out) throws Exception
{
out.write(Integer.parseInt("" + (char)in.get()));
return MessageDecoderResult.OK;
}
}
public static class SessionStub extends DummySession
{
public SessionStub(boolean fragmented)
{
setTransportMetadata(
new DefaultTransportMetadata(
"nio", "socket", false, fragmented,
InetSocketAddress.class,
SocketSessionConfig.class,
IoBuffer.class, FileRegion.class)
);
}
}
Re: Potential DemuxingProtocolDecoderBug
Posted by James Talmage <ja...@jrtechnical.com>.
Bug submitted DIRMINA-672
On Fri, Mar 13, 2009 at 1:54 PM, James Talmage <
james.talmage@jrtechnical.com> wrote:
> I'm using 2.0.0-M4. Upon inspection of the source code, I can tell it's
> going to be a JDK / OS independent issue. Also upon inspection, I've
> discovered the bug is actually in the CumulativeProtocolDecoder (starting @
> line 125):
>
> if (!session.getTransportMetadata().hasFragmentation()) {
> doDecode(session, in, out);
> return;
> }
>
> This breaks the contract with the doDecode method as it is only called once
> (the documentation says it should be called repeatedly until it returns
> false). The following changes makes my previous test case pass, but it's
> probably a little simplistic.
>
> if (!session.getTransportMetadata().hasFragmentation()) {
> while(in.hasRemaining() && doDecode(session, in, out)){
> //Do nothing
> }
> return;
> }
>
> The code should probably make sure that if doDecode returns true, some of
> the buffer was actually consumed (as the code for fragmented transports
> does). Also, it may make sense to provide another method (i.e.
> finishNonFragmentedDecode(...)), for handling the remainder of the buffer
> after doDecode returns false.
>
> I see where the author was headed with this code. Transports (such as UDP)
> that don't support fragmentation probably don't jive with the concept of an
> accumulating buffer (what do we do with the accumulation buffer if we drop a
> UDP packet?). It does however make perfect sense to use such transports
> with the concept of a DemuxingProtocolDecoder. Perhaps it would be better
> to refactor the DemuxingProtocolDecoder so that it's not a subclass of
> CumulativeProtocolDecoder. Create a helper class that handles the fragment
> accumulation aspect. CumulativeProtocolDecoder would always use said helper
> class (throwing an error if the protocol doesn't support fragmentation), but
> DemuxingProtocolDecoder could opt to use it depending on the protocol it
> encounters.
>
> As for JIRA: I tried to submit there first, but it had been a long time
> since I'd used it to submit a bug, and couldn't figure out how to create a
> new issue. I see it now, so I'll do that as well.
>
>
>
> On Fri, Mar 13, 2009 at 7:18 AM, Emmanuel Lecharny <el...@apache.org>wrote:
>
>> Maarten Bosteels wrote:
>>
>>> and please create a JIRA ticket and attach your test-case.
>>>
>>>
>> Also give some info about the version of MINA you are using, the OS you
>> are running on and the JVM you are using.
>>
>> Reading
>> http://mina.apache.org/contact.html#Contact-BeforePostingYourMessagecould help, too.
>>
>> http://www.catb.org/~esr/faqs/smart-questions.html<http://www.catb.org/%7Eesr/faqs/smart-questions.html>is also a nice article.
>>
>> --
>> --
>> cordialement, regards,
>> Emmanuel Lécharny
>> www.iktek.com
>> directory.apache.org
>>
>>
>>
>
>
> --
> James Talmage
> Owner
> JR Technical
> cell: 248-259-3137
> fax: 248-498-6071
>
--
James Talmage
Owner
JR Technical
cell: 248-259-3137
fax: 248-498-6071
Re: Potential DemuxingProtocolDecoderBug
Posted by James Talmage <ja...@jrtechnical.com>.
I'm using 2.0.0-M4. Upon inspection of the source code, I can tell it's
going to be a JDK / OS independent issue. Also upon inspection, I've
discovered the bug is actually in the CumulativeProtocolDecoder (starting @
line 125):
if (!session.getTransportMetadata().hasFragmentation()) {
doDecode(session, in, out);
return;
}
This breaks the contract with the doDecode method as it is only called once
(the documentation says it should be called repeatedly until it returns
false). The following changes makes my previous test case pass, but it's
probably a little simplistic.
if (!session.getTransportMetadata().hasFragmentation()) {
while(in.hasRemaining() && doDecode(session, in, out)){
//Do nothing
}
return;
}
The code should probably make sure that if doDecode returns true, some of
the buffer was actually consumed (as the code for fragmented transports
does). Also, it may make sense to provide another method (i.e.
finishNonFragmentedDecode(...)), for handling the remainder of the buffer
after doDecode returns false.
I see where the author was headed with this code. Transports (such as UDP)
that don't support fragmentation probably don't jive with the concept of an
accumulating buffer (what do we do with the accumulation buffer if we drop a
UDP packet?). It does however make perfect sense to use such transports
with the concept of a DemuxingProtocolDecoder. Perhaps it would be better
to refactor the DemuxingProtocolDecoder so that it's not a subclass of
CumulativeProtocolDecoder. Create a helper class that handles the fragment
accumulation aspect. CumulativeProtocolDecoder would always use said helper
class (throwing an error if the protocol doesn't support fragmentation), but
DemuxingProtocolDecoder could opt to use it depending on the protocol it
encounters.
As for JIRA: I tried to submit there first, but it had been a long time
since I'd used it to submit a bug, and couldn't figure out how to create a
new issue. I see it now, so I'll do that as well.
On Fri, Mar 13, 2009 at 7:18 AM, Emmanuel Lecharny <el...@apache.org>wrote:
> Maarten Bosteels wrote:
>
>> and please create a JIRA ticket and attach your test-case.
>>
>>
> Also give some info about the version of MINA you are using, the OS you are
> running on and the JVM you are using.
>
> Reading
> http://mina.apache.org/contact.html#Contact-BeforePostingYourMessage could
> help, too.
>
> http://www.catb.org/~esr/faqs/smart-questions.html<http://www.catb.org/%7Eesr/faqs/smart-questions.html>is also a nice article.
>
> --
> --
> cordialement, regards,
> Emmanuel Lécharny
> www.iktek.com
> directory.apache.org
>
>
>
--
James Talmage
Owner
JR Technical
cell: 248-259-3137
fax: 248-498-6071
Re: Potential DemuxingProtocolDecoderBug
Posted by Emmanuel Lecharny <el...@apache.org>.
Maarten Bosteels wrote:
> and please create a JIRA ticket and attach your test-case.
>
Also give some info about the version of MINA you are using, the OS you
are running on and the JVM you are using.
Reading
http://mina.apache.org/contact.html#Contact-BeforePostingYourMessage
could help, too.
http://www.catb.org/~esr/faqs/smart-questions.html is also a nice article.
--
--
cordialement, regards,
Emmanuel Lécharny
www.iktek.com
directory.apache.org
Re: Potential DemuxingProtocolDecoderBug
Posted by Maarten Bosteels <mb...@gmail.com>.
and please create a JIRA ticket and attach your test-case.
Maarten
On Fri, Mar 13, 2009 at 9:19 AM, Emmanuel Lecharny <el...@apache.org>wrote:
> James Talmage wrote:
>
>> See test case for explanation
>>
>>
> Can you please give us a clue about what goes wrong with your tests ?
>
> --
> --
> cordialement, regards,
> Emmanuel Lécharny
> www.iktek.com
> directory.apache.org
>
>
>
Re: Potential DemuxingProtocolDecoderBug
Posted by Emmanuel Lecharny <el...@apache.org>.
James Talmage wrote:
> See test case for explanation
>
Can you please give us a clue about what goes wrong with your tests ?
--
--
cordialement, regards,
Emmanuel Lécharny
www.iktek.com
directory.apache.org