You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by Gaston Dombiak <ga...@jivesoftware.com> on 2008/05/16 19:29:46 UTC
What do you think of this proposed change?
An Openfire community member contributed this change regarding a problem he was having in Openfire. I think I like this fix since the CumulativeProtocolDecoder is not delegating to subclasses to correctly handle decoding when there is nothing to decode. What do you think? BTW, this is for MINA 1.1.7.
Index: mina-1.1.7/core/src/main/java/org/apache/mina/filter/codec/CumulativeProtocolDecoder.java
===================================================================
--- mina-1.1.7/core/src/main/java/org/apache/mina/filter/codec/CumulativeProtocolDecoder.java (revision 1407)
+++ mina-1.1.7/core/src/main/java/org/apache/mina/filter/codec/CumulativeProtocolDecoder.java (revision 1412)
@@ -128,20 +128,12 @@
usingSessionBuffer = false;
}
- for (;;) {
+ boolean decoded = true;
+ for (;buf.hasRemaining() && decoded;) {
int oldPos = buf.position();
- boolean decoded = doDecode(session, buf, out);
- if (decoded) {
- if (buf.position() == oldPos) {
- throw new IllegalStateException(
- "doDecode() can't return true when buffer is not consumed.");
- }
-
- if (!buf.hasRemaining()) {
- break;
- }
- } else {
- break;
+ decoded = doDecode(session, buf, out);
+ if (decoded && buf.position() == oldPos) {
+ throw new IllegalStateException("doDecode() can't return true when buffer is not consumed.");
}
}
Thanks,
-- Gato
Re: What do you think of this proposed change?
Posted by Gaston Dombiak <ga...@jivesoftware.com>.
Hey Trustin,
Latest version we were using was 1.1.6 so I’m now checking the changes made for 1.1.7 and later. I found that IoServiceListenerSupport in line 164 is using ifAbsentPut. BTW, that brought some flashbacks from my Smalltalk dev experience. :) However, in Java the implementation is not very optimal since it is always “executing” the missing value. What I mean is that a new synchronized set is always created even if a value already exists in the collection. I haven’t profiled MINA but this is a common place where creating objects is expensive and can be prevented. It may be possible that a standard #get-if-null-then-put operation could be more efficient.
Regarding your latest fix I think it is even better since it is solving the problem at an earlier stage thus avoiding the problem in any decoder not only the cumulative one.
Thanks for your prompt answer.
Regards,
-- Gato
On 5/19/08 6:54 PM, "이희승 (Trustin Lee)" <tr...@gmail.com> wrote:
Hi Gato,
I made sure that no empty buffer is passed as an argument of
ProtocolDecoder by adding additional check in ProtocolCodecFilter. This
is somewhat different fix than you suggested, but it should work, too.
Please svn up and confirm if my fix works for you.
MINA 2 already ignores an empty buffer.
Thanks,
--
Trustin Lee - Principal Software Engineer, JBoss, Red Hat
--
what we call human nature is actually human habit
--
http://gleamynode.net/
Re: What do you think of this proposed change?
Posted by "이희승 (Trustin Lee)" <tr...@gmail.com>.
Hi Gato,
I made sure that no empty buffer is passed as an argument of
ProtocolDecoder by adding additional check in ProtocolCodecFilter. This
is somewhat different fix than you suggested, but it should work, too.
Please svn up and confirm if my fix works for you.
MINA 2 already ignores an empty buffer.
Thanks,
--
Trustin Lee - Principal Software Engineer, JBoss, Red Hat
--
what we call human nature is actually human habit
--
http://gleamynode.net/
Re: What do you think of this proposed change?
Posted by "이희승 (Trustin Lee)" <tr...@gmail.com>.
IIUC, ProtocolCodecFilter is calling ProtocolDecoder.decode() even when
the buffer is empty, right?
On Sat, 17 May 2008 02:29:46 +0900, Gaston Dombiak <ga...@jivesoftware.com>
wrote:
> An Openfire community member contributed this change regarding a problem
> he was having in Openfire. I think I like this fix since the
> CumulativeProtocolDecoder is not delegating to subclasses to correctly
> handle decoding when there is nothing to decode. What do you think? BTW,
> this is for MINA 1.1.7.
>
> Index:
> mina-1.1.7/core/src/main/java/org/apache/mina/filter/codec/CumulativeProtocolDecoder.java
> ===================================================================
> ---
> mina-1.1.7/core/src/main/java/org/apache/mina/filter/codec/CumulativeProtocolDecoder.java
> (revision 1407)
> +++
> mina-1.1.7/core/src/main/java/org/apache/mina/filter/codec/CumulativeProtocolDecoder.java
> (revision 1412)
> @@ -128,20 +128,12 @@
> usingSessionBuffer = false;
> }
>
> - for (;;) {
> + boolean decoded = true;
> + for (;buf.hasRemaining() && decoded;) {
> int oldPos = buf.position();
> - boolean decoded = doDecode(session, buf, out);
> - if (decoded) {
> - if (buf.position() == oldPos) {
> - throw new IllegalStateException(
> - "doDecode() can't return true when buffer
> is not consumed.");
> - }
> -
> - if (!buf.hasRemaining()) {
> - break;
> - }
> - } else {
> - break;
> + decoded = doDecode(session, buf, out);
> + if (decoded && buf.position() == oldPos) {
> + throw new IllegalStateException("doDecode() can't
> return true when buffer is not consumed.");
> }
> }
>
> Thanks,
>
> -- Gato
>
--
Trustin Lee - Principal Software Engineer, JBoss, Red Hat
--
what we call human nature is actually human habit
--
http://gleamynode.net/