You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by "James Talmage (JIRA)" <ji...@apache.org> on 2009/03/13 19:27:52 UTC
[jira] Created: (DIRMINA-672) CumulativeProtocolDecoder/
DemuxingProtocolDecoder
CumulativeProtocolDecoder/ DemuxingProtocolDecoder
--------------------------------------------------
Key: DIRMINA-672
URL: https://issues.apache.org/jira/browse/DIRMINA-672
Project: MINA
Issue Type: Bug
Components: Filter
Affects Versions: 2.0.0-M4, 2.0.0-M3, 2.0.0-M2, 2.0.0-M1
Environment: JDK / OS independent
Reporter: James Talmage
Excerpt from forum discussion at: http://www.nabble.com/Potential-DemuxingProtocolDecoderBug-td22489974.html
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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DIRMINA-672) CumulativeProtocolDecoder/
DemuxingProtocolDecoder
Posted by "James Talmage (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-672?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
James Talmage updated DIRMINA-672:
----------------------------------
Attachment: DemuxingProtocolDecoderBugTest.java
Test case that shows how this issue impacts the DemuxingProtocolDecoder class.
> CumulativeProtocolDecoder/ DemuxingProtocolDecoder
> --------------------------------------------------
>
> Key: DIRMINA-672
> URL: https://issues.apache.org/jira/browse/DIRMINA-672
> Project: MINA
> Issue Type: Bug
> Components: Filter
> Affects Versions: 2.0.0-M1, 2.0.0-M2, 2.0.0-M3, 2.0.0-M4
> Environment: JDK / OS independent
> Reporter: James Talmage
> Attachments: DemuxingProtocolDecoderBugTest.java
>
>
> Excerpt from forum discussion at: http://www.nabble.com/Potential-DemuxingProtocolDecoderBug-td22489974.html
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Reopened: (DIRMINA-672) CumulativeProtocolDecoder/
DemuxingProtocolDecoder
Posted by "James Talmage (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-672?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
James Talmage reopened DIRMINA-672:
-----------------------------------
Current patch will loop endlessly if it's a nonFragmented transport and doDecode never fully consumes the buffer (i.e. it wants to discard the remainder of the buffer).
Take the "CrLfTerminatedCommandLineDecoder" example from the JavaDoc. If you send it "Line1\r\nLine2\r\nLine3" it will parse the first two lines and loop endlessly. We should check the result of doDecode to make sure it wants us to continue.
The fragmented transport code also checks to make sure that some of the buffer is consumed if doDecode returns true (this would likely mean that the doDecode method had a bug, but we should probably check for it and throw an exception)
> CumulativeProtocolDecoder/ DemuxingProtocolDecoder
> --------------------------------------------------
>
> Key: DIRMINA-672
> URL: https://issues.apache.org/jira/browse/DIRMINA-672
> Project: MINA
> Issue Type: Bug
> Components: Filter
> Affects Versions: 2.0.0-M1, 2.0.0-M2, 2.0.0-M3, 2.0.0-M4
> Environment: JDK / OS independent
> Reporter: James Talmage
> Assignee: Emmanuel Lecharny
> Fix For: 2.0.0-RC1
>
> Attachments: DemuxingProtocolDecoderBugTest.java
>
>
> Excerpt from forum discussion at: http://www.nabble.com/Potential-DemuxingProtocolDecoderBug-td22489974.html
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (DIRMINA-672) CumulativeProtocolDecoder/
DemuxingProtocolDecoder
Posted by "Emmanuel Lecharny (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-672?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Emmanuel Lecharny resolved DIRMINA-672.
---------------------------------------
Resolution: Fixed
As far as I remember, the bug has been marked as fix, reopened because of an error in the patch I applied, but should now be considered as closed.
> CumulativeProtocolDecoder/ DemuxingProtocolDecoder
> --------------------------------------------------
>
> Key: DIRMINA-672
> URL: https://issues.apache.org/jira/browse/DIRMINA-672
> Project: MINA
> Issue Type: Bug
> Components: Filter
> Affects Versions: 2.0.0-M1, 2.0.0-M2, 2.0.0-M3, 2.0.0-M4
> Environment: JDK / OS independent
> Reporter: James Talmage
> Assignee: Emmanuel Lecharny
> Fix For: 2.0.0-RC1
>
> Attachments: DemuxingProtocolDecoderBugTest.java
>
>
> Excerpt from forum discussion at: http://www.nabble.com/Potential-DemuxingProtocolDecoderBug-td22489974.html
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DIRMINA-672) CumulativeProtocolDecoder/
DemuxingProtocolDecoder
Posted by "Julien Vermillard (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-672?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Julien Vermillard updated DIRMINA-672:
--------------------------------------
Fix Version/s: (was: 2.0.0-M5)
2.0.0-RC1
> CumulativeProtocolDecoder/ DemuxingProtocolDecoder
> --------------------------------------------------
>
> Key: DIRMINA-672
> URL: https://issues.apache.org/jira/browse/DIRMINA-672
> Project: MINA
> Issue Type: Bug
> Components: Filter
> Affects Versions: 2.0.0-M1, 2.0.0-M2, 2.0.0-M3, 2.0.0-M4
> Environment: JDK / OS independent
> Reporter: James Talmage
> Assignee: Emmanuel Lecharny
> Fix For: 2.0.0-RC1
>
> Attachments: DemuxingProtocolDecoderBugTest.java
>
>
> Excerpt from forum discussion at: http://www.nabble.com/Potential-DemuxingProtocolDecoderBug-td22489974.html
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DIRMINA-672) CumulativeProtocolDecoder/
DemuxingProtocolDecoder
Posted by "Emmanuel Lecharny (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-672?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12683654#action_12683654 ]
Emmanuel Lecharny commented on DIRMINA-672:
-------------------------------------------
It seems pretty obvious that there is a problem :)
I'm testing the test and patching the code. Thanks !
> CumulativeProtocolDecoder/ DemuxingProtocolDecoder
> --------------------------------------------------
>
> Key: DIRMINA-672
> URL: https://issues.apache.org/jira/browse/DIRMINA-672
> Project: MINA
> Issue Type: Bug
> Components: Filter
> Affects Versions: 2.0.0-M1, 2.0.0-M2, 2.0.0-M3, 2.0.0-M4
> Environment: JDK / OS independent
> Reporter: James Talmage
> Assignee: Emmanuel Lecharny
> Fix For: 2.0.0-RC1
>
> Attachments: DemuxingProtocolDecoderBugTest.java
>
>
> Excerpt from forum discussion at: http://www.nabble.com/Potential-DemuxingProtocolDecoderBug-td22489974.html
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Assigned: (DIRMINA-672) CumulativeProtocolDecoder/
DemuxingProtocolDecoder
Posted by "Emmanuel Lecharny (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-672?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Emmanuel Lecharny reassigned DIRMINA-672:
-----------------------------------------
Assignee: Emmanuel Lecharny
> CumulativeProtocolDecoder/ DemuxingProtocolDecoder
> --------------------------------------------------
>
> Key: DIRMINA-672
> URL: https://issues.apache.org/jira/browse/DIRMINA-672
> Project: MINA
> Issue Type: Bug
> Components: Filter
> Affects Versions: 2.0.0-M1, 2.0.0-M2, 2.0.0-M3, 2.0.0-M4
> Environment: JDK / OS independent
> Reporter: James Talmage
> Assignee: Emmanuel Lecharny
> Fix For: 2.0.0-RC1
>
> Attachments: DemuxingProtocolDecoderBugTest.java
>
>
> Excerpt from forum discussion at: http://www.nabble.com/Potential-DemuxingProtocolDecoderBug-td22489974.html
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (DIRMINA-672) CumulativeProtocolDecoder/
DemuxingProtocolDecoder
Posted by "Emmanuel Lecharny (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-672?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Emmanuel Lecharny resolved DIRMINA-672.
---------------------------------------
Resolution: Fixed
Ok, patch applied, test added.
Thanks James !
http://svn.apache.org/viewvc?rev=756242&view=rev
> CumulativeProtocolDecoder/ DemuxingProtocolDecoder
> --------------------------------------------------
>
> Key: DIRMINA-672
> URL: https://issues.apache.org/jira/browse/DIRMINA-672
> Project: MINA
> Issue Type: Bug
> Components: Filter
> Affects Versions: 2.0.0-M1, 2.0.0-M2, 2.0.0-M3, 2.0.0-M4
> Environment: JDK / OS independent
> Reporter: James Talmage
> Assignee: Emmanuel Lecharny
> Fix For: 2.0.0-RC1
>
> Attachments: DemuxingProtocolDecoderBugTest.java
>
>
> Excerpt from forum discussion at: http://www.nabble.com/Potential-DemuxingProtocolDecoderBug-td22489974.html
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DIRMINA-672) CumulativeProtocolDecoder/
DemuxingProtocolDecoder
Posted by "Emmanuel Lecharny (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-672?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Emmanuel Lecharny updated DIRMINA-672:
--------------------------------------
Fix Version/s: (was: 2.0.0-RC1)
2.0.0-M5
> CumulativeProtocolDecoder/ DemuxingProtocolDecoder
> --------------------------------------------------
>
> Key: DIRMINA-672
> URL: https://issues.apache.org/jira/browse/DIRMINA-672
> Project: MINA
> Issue Type: Bug
> Components: Filter
> Affects Versions: 2.0.0-M1, 2.0.0-M2, 2.0.0-M3, 2.0.0-M4
> Environment: JDK / OS independent
> Reporter: James Talmage
> Assignee: Emmanuel Lecharny
> Fix For: 2.0.0-M5
>
> Attachments: DemuxingProtocolDecoderBugTest.java
>
>
> Excerpt from forum discussion at: http://www.nabble.com/Potential-DemuxingProtocolDecoderBug-td22489974.html
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DIRMINA-672) CumulativeProtocolDecoder/
DemuxingProtocolDecoder
Posted by "Emmanuel Lecharny (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-672?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12688071#action_12688071 ]
Emmanuel Lecharny commented on DIRMINA-672:
-------------------------------------------
Sorry, my bad. I moved the decode() method call out of the while just for clarity sake, without adding a check to get out of the loop if it returns false. Probably did that too late ...
Fixed in http://svn.apache.org/viewvc?rev=756989&view=rev
PS: I would really appreciate if you can also provide a test with the Crlf decoder, I'm a bit too lazy and busy to add it myself ;)
Thanks !
> CumulativeProtocolDecoder/ DemuxingProtocolDecoder
> --------------------------------------------------
>
> Key: DIRMINA-672
> URL: https://issues.apache.org/jira/browse/DIRMINA-672
> Project: MINA
> Issue Type: Bug
> Components: Filter
> Affects Versions: 2.0.0-M1, 2.0.0-M2, 2.0.0-M3, 2.0.0-M4
> Environment: JDK / OS independent
> Reporter: James Talmage
> Assignee: Emmanuel Lecharny
> Fix For: 2.0.0-RC1
>
> Attachments: DemuxingProtocolDecoderBugTest.java
>
>
> Excerpt from forum discussion at: http://www.nabble.com/Potential-DemuxingProtocolDecoderBug-td22489974.html
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Closed: (DIRMINA-672) CumulativeProtocolDecoder/
DemuxingProtocolDecoder
Posted by "Emmanuel Lecharny (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-672?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Emmanuel Lecharny closed DIRMINA-672.
-------------------------------------
> CumulativeProtocolDecoder/ DemuxingProtocolDecoder
> --------------------------------------------------
>
> Key: DIRMINA-672
> URL: https://issues.apache.org/jira/browse/DIRMINA-672
> Project: MINA
> Issue Type: Bug
> Components: Filter
> Affects Versions: 2.0.0-M1, 2.0.0-M2, 2.0.0-M3, 2.0.0-M4
> Environment: JDK / OS independent
> Reporter: James Talmage
> Assignee: Emmanuel Lecharny
> Fix For: 2.0.0-M5
>
> Attachments: DemuxingProtocolDecoderBugTest.java
>
>
> Excerpt from forum discussion at: http://www.nabble.com/Potential-DemuxingProtocolDecoderBug-td22489974.html
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DIRMINA-672) CumulativeProtocolDecoder/
DemuxingProtocolDecoder
Posted by "Emmanuel Lecharny (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DIRMINA-672?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Emmanuel Lecharny updated DIRMINA-672:
--------------------------------------
Fix Version/s: 2.0.0-RC1
> CumulativeProtocolDecoder/ DemuxingProtocolDecoder
> --------------------------------------------------
>
> Key: DIRMINA-672
> URL: https://issues.apache.org/jira/browse/DIRMINA-672
> Project: MINA
> Issue Type: Bug
> Components: Filter
> Affects Versions: 2.0.0-M1, 2.0.0-M2, 2.0.0-M3, 2.0.0-M4
> Environment: JDK / OS independent
> Reporter: James Talmage
> Fix For: 2.0.0-RC1
>
> Attachments: DemuxingProtocolDecoderBugTest.java
>
>
> Excerpt from forum discussion at: http://www.nabble.com/Potential-DemuxingProtocolDecoderBug-td22489974.html
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.