You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Filip Hanik - Dev Lists <de...@hanik.com> on 2008/02/27 20:51:00 UTC
Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Bill Barker wrote:
> "Remy Maucherat" <re...@apache.org> wrote in message
> news:46CB7F51.1030201@apache.org...
>
>> Filip Hanik - Dev Lists wrote:
>>
>>> Test Case and 5.5.x patch can be found here.
>>> http://people.apache.org/~fhanik/tomcat/b2c/
>>>
>>> This is what is happening
>>>
>>> int cnt=conv.read( result, 0, BUFFER_SIZE );
>>> is called with a "while (true)" statement,
>>>
>>> When the IntermediateInputStream.read returns -1, the above statement
>>> returns cnt==1.
>>> So to avoid calling conv.read, we must check to see if we have more bytes
>>> to read by implementing the available() method, to avoid the inputstream
>>> ever returning -1.
>>>
>> It's possible, but I have a hard time understanding the issue.
>>
>>
>
> The issue is that InputStreamReader reads 8192 bytes from
> IntermediateInputStream on the first go. It then translates them into 2734
> chars, but thinks that the last few bytes represent an incomplete char, so
> holds onto them. On the next call, IntermediateInputStream returns -1, so
> InputStreamReader outputs the last char as best it can (resulting in
> returning 1). Then the IntermediateInputStream buffer is reset, and it can
> continue on reading (but from the wrong position, resulting in corruption).
>
> Filip's patch is inelegant (better would be to use the ByteChunk sink), but
> other than my looking for a better way to do it, I can't come up with the
> required technical reason to porting the base of it to 5.5 (of course, I
> could care less what he does in his sandbox :).
>
unfortunately, the "elegant" solution caused a regression :(
http://issues.apache.org/bugzilla/show_bug.cgi?id=44494
I tested this with the inelegant (original proposed) patch and it worked
fine, so I'm gonna fix this in trunk and propose the patch to 6.0
Filip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
Mark Thomas wrote:
> Filip Hanik - Dev Lists wrote:
>> Mark Thomas wrote:
>>> Remy Maucherat wrote:
>>>> On Thu, 2008-02-28 at 01:36 +0100, Remy Maucherat wrote:
>>>>> Index: java/org/apache/catalina/connector/InputBuffer.java
>>>>> ===================================================================
>>>>> ---java/org/apache/catalina/connector/InputBuffer.java
>>>>> (revision 630535)
>>>>> +++java/org/apache/catalina/connector/InputBuffer.java (working
>>>>> copy)
>>>>> @@ -355,7 +355,7 @@
>>>>> }
>>>>>
>>>>> state = CHAR_STATE;
>>>>> - conv.convert(bb, cb, len);
>>>>> + conv.convert(bb, cb, bb.getLength());
>>>>> bb.setOffset(bb.getEnd());
>>>>>
>>>>> return cb.getLength();
>>>>
>>>> This seems to work for me. It should be equivalent to the value
>>>> returned
>>>> by the available method, since the value passed is the same (it is the
>>>> amount of bytes which have been read).
>>>
>>> Just to be clear, do you mean that the above patch
>>> http://issues.apache.org/bugzilla/show_bug.cgi?id=44494 ?
>> that's correct
>> Filip
>
> First off - sorry it has taken me a few days to reply.
>
> Secondly, I still haven't had enough time to look at this in detail so
> my reasoning may be way off base. It it is, I apologise - feel free to
> correct me with or without flames.
>
> Looking through the InputBuffer code, realReadBytes(), realReadChars()
> and realWriteChars() all ignore the parameters passed in with the
> exception of len in realReadChars() which the patch above changes so
> it too is ignored.
>
> It appears to me that all of these methods need to be modified not to
> take any parameters since they all operate on the internal buffers
> rather than the ones passed to them (a job for another day). On this
> basis, the patch above patch strikes me as the right way forward
> rather than the one currently applied to trunk.
>
> On the basis of the above, I would vote -0 for the current patch since
> I don't think I have done enough research to vote -1 or +1.
>
> However, since the current patch removes a method from a public API I
> will be voting -1 solely on that basis. Once that is fixed, I'll
> change my vote to -0 until I have a change to look at this some more
> (hopefully in the next week or so).
the original patch didn't do that
- while( true ) { // conv.ready() ) {
+ while( iis.available()>0 ) { // conv.ready() ) {
int cnt=conv.read( result, 0, BUFFER_SIZE );
if( cnt <= 0 ) {
// End of stream ! - we may be in a bad state
@@ -251,7 +251,12 @@
public final int read() throws IOException {
return (pos < end ) ? (buf[pos++] & 0xff) : -1;
}
+
+ public int available() throws IOException {
+ return end-pos;
the API change is because a new API was introduced in the refactoring
Filip
+ }
>
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Mark Thomas <ma...@apache.org>.
Costin Manolache wrote:
> Sorry, I didn't mean for 5.5.x or 6.0.x, but for whatever is next.
API breaking development is in
http://svn.apache.org/repos/asf/tomcat/trunk/ (not that there is that much
of it yet). Nearly all patches to that branch (CTR) are proposed for back
porting to 6.0.x, some are proposed for 5.5.x and the odd one makes it all
the way back to 4.1.x.
I don't think we ever decided how 6.2.x/7.0.x would be started. We can
a) start with http://svn.apache.org/repos/asf/tomcat/tc6.0.x/trunk/ and
merge a bunch of patches from http://svn.apache.org/repos/asf/tomcat/trunk/ or
b) just start with http://svn.apache.org/repos/asf/tomcat/trunk/
I suspect we'll go with a) for stability reasons but we aren't at a point
where we need to make that call yet.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Costin Manolache <co...@gmail.com>.
On 3/3/08, Filip Hanik - Dev Lists <de...@hanik.com> wrote:
> Costin Manolache wrote:
> > On 3/3/08, Remy Maucherat <re...@apache.org> wrote:
> >
> >> On Mon, 2008-03-03 at 15:58 -0700, Filip Hanik - Dev Lists wrote:
> >>
> >>
> >>> Remy Maucherat wrote:
> >>>
> >>>> This problem is a small detail. Much more should be done if you want
> >>>>
> >> to
> >>
> >>>> do a refactoring: both the mark functionality and readLine need to
> >>>>
> >> have
> >>
> >>>> direct access to the buffer to be able to be coded in a sane way (and
> >>>>
> >> be
> >>
> >>>> more efficient too).
> >>>>
> >>>>
> >>> yes, so the question is for 6.0.x and 5.5.x, do we wanna proceed down
> >>> the refactor route?
> >>> I was against it in the beginning for the fear of regression. I
> >>> personally think the whole bytechunk/charchunk thing is very complex,
> >>> and can be done easier, but that is something I would play around in
> >>> sandbox, and eventually bring into trunk if it was working.
> >>>
> >> I am not really interested in participating. Besides some possible
> >> simple cleanup, CharChunk is actually too simple rather than too complex
> >> (ByteChunk is just fine, and doesn't need additional features): to
> >> improve, it would need to get mark capabilities and (unfortunately) get
> >> a readLine (it's even more problematic to implement it outside the
> >> class). I am pretty sure using the NIO buffers will be proposed for some
> >> reason, which are horrible to use as far as I am concerned.
> >>
> >
> > I think the byte->char conversion should be cleaned, the InputStream hack
> > was
> > needed 9 years ago because regular conversion sucked and we couldn't use
> > the
> > convertors directly. I've seen quite a few other projects doing the
> > conversion
> > themself at least for UTF8 and 8859-1.
> >
>
> no one disagrees with that. It's just that 5.5.x and 6.0.x is not the
> place for that cleanup.
Sorry, I didn't mean for 5.5.x or 6.0.x, but for whatever is next.
Maybe making coyote more independent would make some sense ?
Costin
>
> > Regarding using NIO buffers - I agree with Remy, the flip()s are horrible,
> > I think
> > there is a version in sandbox that replaces byte[] with the ByteBuffers, and
> > it doesn't
> > seem to be worth it.
> > However I think it would be just great to start making a distinction between
> > 'buffer' and
> > 'pointer to a buffer' - IMO that's the main cause of complexity.
>
> absolutely.
>
> > And maybe
> > add
> > 'implements CharSequence, Appendable' to make the coyote classes more
> > friendly for
> > direct use.
> >
> >
> >
> >> for 6.0.x and 5.5.x, I'd rather keep the fixes to the actual bug fix to
> >>
> >>> maintain stability
> >>>
> >> There's no way this sort of work could be good for these branches.
> >>
> >
> > Since I'm quite out of sync - what is the current 'dev branch' and is there
> > any
> > 'some API changes allowed' release planned ?
> > Costin
> >
> >
> > Rémy
> >
> >>
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> >> For additional commands, e-mail: dev-help@tomcat.apache.org
> >>
> >>
> >>
>
> >> ------------------------------------------------------------------------
> >>
> >> No virus found in this incoming message.
> >> Checked by AVG Free Edition.
> >> Version: 7.5.516 / Virus Database: 269.21.3/1307 - Release Date: 3/2/2008 3:59 PM
>
> >>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
Costin Manolache wrote:
> On 3/3/08, Remy Maucherat <re...@apache.org> wrote:
>
>> On Mon, 2008-03-03 at 15:58 -0700, Filip Hanik - Dev Lists wrote:
>>
>>
>>> Remy Maucherat wrote:
>>>
>>>> This problem is a small detail. Much more should be done if you want
>>>>
>> to
>>
>>>> do a refactoring: both the mark functionality and readLine need to
>>>>
>> have
>>
>>>> direct access to the buffer to be able to be coded in a sane way (and
>>>>
>> be
>>
>>>> more efficient too).
>>>>
>>>>
>>> yes, so the question is for 6.0.x and 5.5.x, do we wanna proceed down
>>> the refactor route?
>>> I was against it in the beginning for the fear of regression. I
>>> personally think the whole bytechunk/charchunk thing is very complex,
>>> and can be done easier, but that is something I would play around in
>>> sandbox, and eventually bring into trunk if it was working.
>>>
>> I am not really interested in participating. Besides some possible
>> simple cleanup, CharChunk is actually too simple rather than too complex
>> (ByteChunk is just fine, and doesn't need additional features): to
>> improve, it would need to get mark capabilities and (unfortunately) get
>> a readLine (it's even more problematic to implement it outside the
>> class). I am pretty sure using the NIO buffers will be proposed for some
>> reason, which are horrible to use as far as I am concerned.
>>
>
> I think the byte->char conversion should be cleaned, the InputStream hack
> was
> needed 9 years ago because regular conversion sucked and we couldn't use
> the
> convertors directly. I've seen quite a few other projects doing the
> conversion
> themself at least for UTF8 and 8859-1.
>
no one disagrees with that. It's just that 5.5.x and 6.0.x is not the
place for that cleanup.
> Regarding using NIO buffers - I agree with Remy, the flip()s are horrible,
> I think
> there is a version in sandbox that replaces byte[] with the ByteBuffers, and
> it doesn't
> seem to be worth it.
> However I think it would be just great to start making a distinction between
> 'buffer' and
> 'pointer to a buffer' - IMO that's the main cause of complexity.
absolutely.
> And maybe
> add
> 'implements CharSequence, Appendable' to make the coyote classes more
> friendly for
> direct use.
>
>
>
>> for 6.0.x and 5.5.x, I'd rather keep the fixes to the actual bug fix to
>>
>>> maintain stability
>>>
>> There's no way this sort of work could be good for these branches.
>>
>
> Since I'm quite out of sync - what is the current 'dev branch' and is there
> any
> 'some API changes allowed' release planned ?
> Costin
>
>
> Rémy
>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>>
>>
>> ------------------------------------------------------------------------
>>
>> No virus found in this incoming message.
>> Checked by AVG Free Edition.
>> Version: 7.5.516 / Virus Database: 269.21.3/1307 - Release Date: 3/2/2008 3:59 PM
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Costin Manolache <co...@gmail.com>.
On 3/3/08, Remy Maucherat <re...@apache.org> wrote:
>
> On Mon, 2008-03-03 at 15:58 -0700, Filip Hanik - Dev Lists wrote:
>
> > Remy Maucherat wrote:
> > > This problem is a small detail. Much more should be done if you want
> to
> > > do a refactoring: both the mark functionality and readLine need to
> have
> > > direct access to the buffer to be able to be coded in a sane way (and
> be
> > > more efficient too).
> > >
> > yes, so the question is for 6.0.x and 5.5.x, do we wanna proceed down
> > the refactor route?
> > I was against it in the beginning for the fear of regression. I
> > personally think the whole bytechunk/charchunk thing is very complex,
> > and can be done easier, but that is something I would play around in
> > sandbox, and eventually bring into trunk if it was working.
>
>
> I am not really interested in participating. Besides some possible
> simple cleanup, CharChunk is actually too simple rather than too complex
> (ByteChunk is just fine, and doesn't need additional features): to
> improve, it would need to get mark capabilities and (unfortunately) get
> a readLine (it's even more problematic to implement it outside the
> class). I am pretty sure using the NIO buffers will be proposed for some
> reason, which are horrible to use as far as I am concerned.
I think the byte->char conversion should be cleaned, the InputStream hack
was
needed 9 years ago because regular conversion sucked and we couldn't use
the
convertors directly. I've seen quite a few other projects doing the
conversion
themself at least for UTF8 and 8859-1.
Regarding using NIO buffers - I agree with Remy, the flip()s are horrible,
I think
there is a version in sandbox that replaces byte[] with the ByteBuffers, and
it doesn't
seem to be worth it.
However I think it would be just great to start making a distinction between
'buffer' and
'pointer to a buffer' - IMO that's the main cause of complexity. And maybe
add
'implements CharSequence, Appendable' to make the coyote classes more
friendly for
direct use.
> for 6.0.x and 5.5.x, I'd rather keep the fixes to the actual bug fix to
> > maintain stability
>
>
> There's no way this sort of work could be good for these branches.
Since I'm quite out of sync - what is the current 'dev branch' and is there
any
'some API changes allowed' release planned ?
Costin
Rémy
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
Remy Maucherat wrote:
> On Mon, 2008-03-03 at 15:58 -0700, Filip Hanik - Dev Lists wrote:
>
>> Remy Maucherat wrote:
>>
>>> This problem is a small detail. Much more should be done if you want to
>>> do a refactoring: both the mark functionality and readLine need to have
>>> direct access to the buffer to be able to be coded in a sane way (and be
>>> more efficient too).
>>>
>>>
>> yes, so the question is for 6.0.x and 5.5.x, do we wanna proceed down
>> the refactor route?
>> I was against it in the beginning for the fear of regression. I
>> personally think the whole bytechunk/charchunk thing is very complex,
>> and can be done easier, but that is something I would play around in
>> sandbox, and eventually bring into trunk if it was working.
>>
>
> I am not really interested in participating. Besides some possible
> simple cleanup, CharChunk is actually too simple rather than too complex
> (ByteChunk is just fine, and doesn't need additional features): to
> improve, it would need to get mark capabilities and (unfortunately) get
> a readLine (it's even more problematic to implement it outside the
> class). I am pretty sure using the NIO buffers will be proposed for some
> reason, which are horrible to use as far as I am concerned.
>
>
>> for 6.0.x and 5.5.x, I'd rather keep the fixes to the actual bug fix to
>> maintain stability
>>
>
> There's no way this sort of work could be good for these branches.
>
what the above lines meant, is that for 5.5.x and 6.0.x I don't think
any refactoring is in order, its not needed. keep it to the simple bug
fixes, from your response, I understand you took it the other way.
Filip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS:
svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Remy Maucherat <re...@apache.org>.
On Mon, 2008-03-03 at 15:58 -0700, Filip Hanik - Dev Lists wrote:
> Remy Maucherat wrote:
> > This problem is a small detail. Much more should be done if you want to
> > do a refactoring: both the mark functionality and readLine need to have
> > direct access to the buffer to be able to be coded in a sane way (and be
> > more efficient too).
> >
> yes, so the question is for 6.0.x and 5.5.x, do we wanna proceed down
> the refactor route?
> I was against it in the beginning for the fear of regression. I
> personally think the whole bytechunk/charchunk thing is very complex,
> and can be done easier, but that is something I would play around in
> sandbox, and eventually bring into trunk if it was working.
I am not really interested in participating. Besides some possible
simple cleanup, CharChunk is actually too simple rather than too complex
(ByteChunk is just fine, and doesn't need additional features): to
improve, it would need to get mark capabilities and (unfortunately) get
a readLine (it's even more problematic to implement it outside the
class). I am pretty sure using the NIO buffers will be proposed for some
reason, which are horrible to use as far as I am concerned.
> for 6.0.x and 5.5.x, I'd rather keep the fixes to the actual bug fix to
> maintain stability
There's no way this sort of work could be good for these branches.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
Remy Maucherat wrote:
> On Mon, 2008-03-03 at 21:13 +0000, Mark Thomas wrote:
>
>> Filip Hanik - Dev Lists wrote:
>>
>>> Mark Thomas wrote:
>>>
>>>> Remy Maucherat wrote:
>>>>
>>>>> On Thu, 2008-02-28 at 01:36 +0100, Remy Maucherat wrote:
>>>>>
>>>>>> Index: java/org/apache/catalina/connector/InputBuffer.java
>>>>>>
>>>>>>
>> ===================================================================
>>
>>>>>> ---java/org/apache/catalina/connector/InputBuffer.java
>>>>>>
>> (revision
>>
>>>>>> 630535)
>>>>>> +++java/org/apache/catalina/connector/InputBuffer.java
>>>>>>
>> (working
>>
>>>>>> copy)
>>>>>> @@ -355,7 +355,7 @@
>>>>>> }
>>>>>>
>>>>>> state = CHAR_STATE;
>>>>>> - conv.convert(bb, cb, len);
>>>>>> + conv.convert(bb, cb, bb.getLength());
>>>>>> bb.setOffset(bb.getEnd());
>>>>>>
>>>>>> return cb.getLength();
>>>>>>
>>>>> This seems to work for me. It should be equivalent to the value
>>>>>
>> returned
>>
>>>>> by the available method, since the value passed is the same (it is
>>>>>
>> the
>>
>>>>> amount of bytes which have been read).
>>>>>
>>>> Just to be clear, do you mean that the above patch
>>>> http://issues.apache.org/bugzilla/show_bug.cgi?id=44494 ?
>>>>
>>> that's correct
>>> Filip
>>>
>> First off - sorry it has taken me a few days to reply.
>>
>> Secondly, I still haven't had enough time to look at this in detail so
>> my
>> reasoning may be way off base. It it is, I apologise - feel free to
>> correct
>> me with or without flames.
>>
>> Looking through the InputBuffer code, realReadBytes(), realReadChars()
>> and
>> realWriteChars() all ignore the parameters passed in with the
>> exception of
>> len in realReadChars() which the patch above changes so it too is
>> ignored.
>>
>> It appears to me that all of these methods need to be modified not to
>> take
>> any parameters since they all operate on the internal buffers rather
>> than
>> the ones passed to them (a job for another day). On this basis, the
>> patch
>> above patch strikes me as the right way forward rather than the one
>> currently applied to trunk.
>>
>
> This problem is a small detail. Much more should be done if you want to
> do a refactoring: both the mark functionality and readLine need to have
> direct access to the buffer to be able to be coded in a sane way (and be
> more efficient too).
>
yes, so the question is for 6.0.x and 5.5.x, do we wanna proceed down
the refactor route?
I was against it in the beginning for the fear of regression. I
personally think the whole bytechunk/charchunk thing is very complex,
and can be done easier, but that is something I would play around in
sandbox, and eventually bring into trunk if it was working.
for 6.0.x and 5.5.x, I'd rather keep the fixes to the actual bug fix to
maintain stability
Filip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn
commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Remy Maucherat <re...@apache.org>.
On Mon, 2008-03-03 at 21:13 +0000, Mark Thomas wrote:
> Filip Hanik - Dev Lists wrote:
> > Mark Thomas wrote:
> >> Remy Maucherat wrote:
> >>> On Thu, 2008-02-28 at 01:36 +0100, Remy Maucherat wrote:
> >>>> Index: java/org/apache/catalina/connector/InputBuffer.java
> >>>>
> ===================================================================
> >>>> ---java/org/apache/catalina/connector/InputBuffer.java
> (revision
> >>>> 630535)
> >>>> +++java/org/apache/catalina/connector/InputBuffer.java
> (working
> >>>> copy)
> >>>> @@ -355,7 +355,7 @@
> >>>> }
> >>>>
> >>>> state = CHAR_STATE;
> >>>> - conv.convert(bb, cb, len);
> >>>> + conv.convert(bb, cb, bb.getLength());
> >>>> bb.setOffset(bb.getEnd());
> >>>>
> >>>> return cb.getLength();
> >>>
> >>> This seems to work for me. It should be equivalent to the value
> returned
> >>> by the available method, since the value passed is the same (it is
> the
> >>> amount of bytes which have been read).
> >>
> >> Just to be clear, do you mean that the above patch
> >> http://issues.apache.org/bugzilla/show_bug.cgi?id=44494 ?
> > that's correct
> > Filip
>
> First off - sorry it has taken me a few days to reply.
>
> Secondly, I still haven't had enough time to look at this in detail so
> my
> reasoning may be way off base. It it is, I apologise - feel free to
> correct
> me with or without flames.
>
> Looking through the InputBuffer code, realReadBytes(), realReadChars()
> and
> realWriteChars() all ignore the parameters passed in with the
> exception of
> len in realReadChars() which the patch above changes so it too is
> ignored.
>
> It appears to me that all of these methods need to be modified not to
> take
> any parameters since they all operate on the internal buffers rather
> than
> the ones passed to them (a job for another day). On this basis, the
> patch
> above patch strikes me as the right way forward rather than the one
> currently applied to trunk.
This problem is a small detail. Much more should be done if you want to
do a refactoring: both the mark functionality and readLine need to have
direct access to the buffer to be able to be coded in a sane way (and be
more efficient too).
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Mark Thomas <ma...@apache.org>.
Filip Hanik - Dev Lists wrote:
> Mark Thomas wrote:
>> Remy Maucherat wrote:
>>> On Thu, 2008-02-28 at 01:36 +0100, Remy Maucherat wrote:
>>>> Index: java/org/apache/catalina/connector/InputBuffer.java
>>>> ===================================================================
>>>> ---java/org/apache/catalina/connector/InputBuffer.java (revision
>>>> 630535)
>>>> +++java/org/apache/catalina/connector/InputBuffer.java (working
>>>> copy)
>>>> @@ -355,7 +355,7 @@
>>>> }
>>>>
>>>> state = CHAR_STATE;
>>>> - conv.convert(bb, cb, len);
>>>> + conv.convert(bb, cb, bb.getLength());
>>>> bb.setOffset(bb.getEnd());
>>>>
>>>> return cb.getLength();
>>>
>>> This seems to work for me. It should be equivalent to the value returned
>>> by the available method, since the value passed is the same (it is the
>>> amount of bytes which have been read).
>>
>> Just to be clear, do you mean that the above patch
>> http://issues.apache.org/bugzilla/show_bug.cgi?id=44494 ?
> that's correct
> Filip
First off - sorry it has taken me a few days to reply.
Secondly, I still haven't had enough time to look at this in detail so my
reasoning may be way off base. It it is, I apologise - feel free to correct
me with or without flames.
Looking through the InputBuffer code, realReadBytes(), realReadChars() and
realWriteChars() all ignore the parameters passed in with the exception of
len in realReadChars() which the patch above changes so it too is ignored.
It appears to me that all of these methods need to be modified not to take
any parameters since they all operate on the internal buffers rather than
the ones passed to them (a job for another day). On this basis, the patch
above patch strikes me as the right way forward rather than the one
currently applied to trunk.
On the basis of the above, I would vote -0 for the current patch since I
don't think I have done enough research to vote -1 or +1.
However, since the current patch removes a method from a public API I will
be voting -1 solely on that basis. Once that is fixed, I'll change my vote
to -0 until I have a change to look at this some more (hopefully in the
next week or so).
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
Mark Thomas wrote:
> Remy Maucherat wrote:
>> On Thu, 2008-02-28 at 01:36 +0100, Remy Maucherat wrote:
>>> Index: java/org/apache/catalina/connector/InputBuffer.java
>>> ===================================================================
>>> ---java/org/apache/catalina/connector/InputBuffer.java (revision
>>> 630535)
>>> +++java/org/apache/catalina/connector/InputBuffer.java (working
>>> copy)
>>> @@ -355,7 +355,7 @@
>>> }
>>>
>>> state = CHAR_STATE;
>>> - conv.convert(bb, cb, len);
>>> + conv.convert(bb, cb, bb.getLength());
>>> bb.setOffset(bb.getEnd());
>>>
>>> return cb.getLength();
>>
>> This seems to work for me. It should be equivalent to the value returned
>> by the available method, since the value passed is the same (it is the
>> amount of bytes which have been read).
>
> Just to be clear, do you mean that the above patch
> http://issues.apache.org/bugzilla/show_bug.cgi?id=44494 ?
that's correct
Filip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Mark Thomas <ma...@apache.org>.
Remy Maucherat wrote:
> On Thu, 2008-02-28 at 01:36 +0100, Remy Maucherat wrote:
>> Index: java/org/apache/catalina/connector/InputBuffer.java
>> ===================================================================
>> ---java/org/apache/catalina/connector/InputBuffer.java (revision 630535)
>> +++java/org/apache/catalina/connector/InputBuffer.java (working copy)
>> @@ -355,7 +355,7 @@
>> }
>>
>> state = CHAR_STATE;
>> - conv.convert(bb, cb, len);
>> + conv.convert(bb, cb, bb.getLength());
>> bb.setOffset(bb.getEnd());
>>
>> return cb.getLength();
>
> This seems to work for me. It should be equivalent to the value returned
> by the available method, since the value passed is the same (it is the
> amount of bytes which have been read).
Just to be clear, do you mean that the above patch
http://issues.apache.org/bugzilla/show_bug.cgi?id=44494 ?
Cheers,
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit:
r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Remy Maucherat <re...@apache.org>.
On Thu, 2008-02-28 at 01:36 +0100, Remy Maucherat wrote:
> Index: java/org/apache/catalina/connector/InputBuffer.java
> ===================================================================
> ---java/org/apache/catalina/connector/InputBuffer.java (revision 630535)
> +++java/org/apache/catalina/connector/InputBuffer.java (working copy)
> @@ -355,7 +355,7 @@
> }
>
> state = CHAR_STATE;
> - conv.convert(bb, cb, len);
> + conv.convert(bb, cb, bb.getLength());
> bb.setOffset(bb.getEnd());
>
> return cb.getLength();
This seems to work for me. It should be equivalent to the value returned
by the available method, since the value passed is the same (it is the
amount of bytes which have been read).
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
Remy Maucherat wrote:
> On Wed, 2008-02-27 at 18:25 -0700, Filip Hanik - Dev Lists wrote:
>
>> Remy Maucherat wrote:
>>
>>> On Wed, 2008-02-27 at 17:08 -0700, Filip Hanik - Dev Lists wrote:
>>>
>>>
>>>> Remy Maucherat wrote:
>>>>
>>>>
>>>>> This makes me think there's a straight issue in CharChunk (I suppose the
>>>>> problem is that realReadChars gets called with len = 0; if it handled
>>>>> that scenario, things should be fine).
>>>>>
>>>>>
>>>>>
>>>> the old char chunk worked just fine, so why not stick with it. the error
>>>> above is a result of B2CConverter and InputBuffer being modified
>>>>
>>>>
>>> Ok, but the new code looks a bit nicer,
>>>
>> huh? here we go again, first it was "elegant", now "it's nice". how
>> about "it works" as argument.
>> bottom line is, my patch was very simple, the B2C Converter is using an
>> intermediary inputstream, but wasn't looking if there was data available
>> to read, so that
>>
>> take a look at the original patch proposed
>> http://people.apache.org/~fhanik/tomcat/b2c/patch.txt
>>
>> obviously, my patch touches nothing of what you claim to be "my code",
>> all I did today was, revert the old patch, and apply the simple patch.
>> and it works.
>>
>>
>>> so I would prefer keeping it if
>>> possible. From what I can see, your code replaces the limit value (the
>>> one which would cause the problem if it was 0) by the amount of bytes
>>> which have been read in the ByteChunk. This would translate to:
>>> Index: java/org/apache/catalina/connector/InputBuffer.java
>>> ===================================================================
>>> ---java/org/apache/catalina/connector/InputBuffer.java (revision 630535)
>>> +++java/org/apache/catalina/connector/InputBuffer.java (working copy)
>>> @@ -355,7 +355,7 @@
>>> }
>>>
>>> state = CHAR_STATE;
>>> - conv.convert(bb, cb, len);
>>> + conv.convert(bb, cb, bb.getLength());
>>> bb.setOffset(bb.getEnd());
>>>
>>> return cb.getLength();
>>>
>>> The problem is that I sort of like the idea of the limit (it's related
>>> to the space left in the char buffer for adding the parsed chars). So
>>> maybe trying to respect that with a decent fallback could work (didn't
>>> try it):
>>> Index: java/org/apache/catalina/connector/InputBuffer.java
>>> ===================================================================
>>> ---java/org/apache/catalina/connector/InputBuffer.java (revision 630535)
>>> +++java/org/apache/catalina/connector/InputBuffer.java (working copy)
>>> @@ -355,7 +355,7 @@
>>> }
>>>
>>> state = CHAR_STATE;
>>> - conv.convert(bb, cb, len);
>>> + conv.convert(bb, cb, (len == 0) ? bb.getLength() : len);
>>> bb.setOffset(bb.getEnd());
>>>
>>> return cb.getLength();
>>>
>>>
>> so now we're trying to "fix the fix". why is that? I do NOT claim that
>> "my shit don't stink" but lets face it, the broken patch came with such
>> claim. and now were arguing that it's "nicer".
>> sorry for being blunt, but lets cut to the chase here, what is it that
>> you are really trying to say and maybe we can focus the technical
>> discussion on that, instead of running around in circles.
>>
>
> I don't see anything wrong with fixing the thing.
>
>
>> Obviously the whole byte/char-chunk code is very sensitive, since
>> several components keep their own offset/end/limit flags to the same
>> byte[], which makes it somewhat of a difficult thing to read into. and
>> that's why I managed to come up with a patch that didn't upset that eco
>> system of the old code.
>>
>
> I agree, but I don't see this is being proposed. It could be too risky
> obviously.
>
not really, the patch was fairly large, and I was against for the reason
of regression when the patch becomes a redo/refactor of how the logic
works. especially when there is a much simpler fix that does the exact
thing.
http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java?r1=572497&r2=572496&pathrev=572497
and the comment that went along with the checkin
"The patch I just committed is still a bit fragile (since it depends on
InputBuffer knowing what CharChunk does internally)"
at this point I don't see how this has to be argued again. the series of
events speak for themselves.
And Remy, I appreciate how you've handled this, you've kept your cool, I
had a rough day yesterday, and I apologize if I snapped in my email. Not
my intention.
Filip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn
commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Remy Maucherat <re...@apache.org>.
On Wed, 2008-02-27 at 18:25 -0700, Filip Hanik - Dev Lists wrote:
> Remy Maucherat wrote:
> > On Wed, 2008-02-27 at 17:08 -0700, Filip Hanik - Dev Lists wrote:
> >
> >> Remy Maucherat wrote:
> >>
> >>> This makes me think there's a straight issue in CharChunk (I suppose the
> >>> problem is that realReadChars gets called with len = 0; if it handled
> >>> that scenario, things should be fine).
> >>>
> >>>
> >> the old char chunk worked just fine, so why not stick with it. the error
> >> above is a result of B2CConverter and InputBuffer being modified
> >>
> >
> > Ok, but the new code looks a bit nicer,
> huh? here we go again, first it was "elegant", now "it's nice". how
> about "it works" as argument.
> bottom line is, my patch was very simple, the B2C Converter is using an
> intermediary inputstream, but wasn't looking if there was data available
> to read, so that
>
> take a look at the original patch proposed
> http://people.apache.org/~fhanik/tomcat/b2c/patch.txt
>
> obviously, my patch touches nothing of what you claim to be "my code",
> all I did today was, revert the old patch, and apply the simple patch.
> and it works.
>
> > so I would prefer keeping it if
> > possible. From what I can see, your code replaces the limit value (the
> > one which would cause the problem if it was 0) by the amount of bytes
> > which have been read in the ByteChunk. This would translate to:
> > Index: java/org/apache/catalina/connector/InputBuffer.java
> > ===================================================================
> > ---java/org/apache/catalina/connector/InputBuffer.java (revision 630535)
> > +++java/org/apache/catalina/connector/InputBuffer.java (working copy)
> > @@ -355,7 +355,7 @@
> > }
> >
> > state = CHAR_STATE;
> > - conv.convert(bb, cb, len);
> > + conv.convert(bb, cb, bb.getLength());
> > bb.setOffset(bb.getEnd());
> >
> > return cb.getLength();
> >
> > The problem is that I sort of like the idea of the limit (it's related
> > to the space left in the char buffer for adding the parsed chars). So
> > maybe trying to respect that with a decent fallback could work (didn't
> > try it):
> > Index: java/org/apache/catalina/connector/InputBuffer.java
> > ===================================================================
> > ---java/org/apache/catalina/connector/InputBuffer.java (revision 630535)
> > +++java/org/apache/catalina/connector/InputBuffer.java (working copy)
> > @@ -355,7 +355,7 @@
> > }
> >
> > state = CHAR_STATE;
> > - conv.convert(bb, cb, len);
> > + conv.convert(bb, cb, (len == 0) ? bb.getLength() : len);
> > bb.setOffset(bb.getEnd());
> >
> > return cb.getLength();
> >
> so now we're trying to "fix the fix". why is that? I do NOT claim that
> "my shit don't stink" but lets face it, the broken patch came with such
> claim. and now were arguing that it's "nicer".
> sorry for being blunt, but lets cut to the chase here, what is it that
> you are really trying to say and maybe we can focus the technical
> discussion on that, instead of running around in circles.
I don't see anything wrong with fixing the thing.
> Obviously the whole byte/char-chunk code is very sensitive, since
> several components keep their own offset/end/limit flags to the same
> byte[], which makes it somewhat of a difficult thing to read into. and
> that's why I managed to come up with a patch that didn't upset that eco
> system of the old code.
I agree, but I don't see this is being proposed. It could be too risky
obviously.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
Remy Maucherat wrote:
> On Wed, 2008-02-27 at 17:08 -0700, Filip Hanik - Dev Lists wrote:
>
>> Remy Maucherat wrote:
>>
>>> This makes me think there's a straight issue in CharChunk (I suppose the
>>> problem is that realReadChars gets called with len = 0; if it handled
>>> that scenario, things should be fine).
>>>
>>>
>> the old char chunk worked just fine, so why not stick with it. the error
>> above is a result of B2CConverter and InputBuffer being modified
>>
>
> Ok, but the new code looks a bit nicer,
huh? here we go again, first it was "elegant", now "it's nice". how
about "it works" as argument.
bottom line is, my patch was very simple, the B2C Converter is using an
intermediary inputstream, but wasn't looking if there was data available
to read, so that
take a look at the original patch proposed
http://people.apache.org/~fhanik/tomcat/b2c/patch.txt
obviously, my patch touches nothing of what you claim to be "my code",
all I did today was, revert the old patch, and apply the simple patch.
and it works.
> so I would prefer keeping it if
> possible. From what I can see, your code replaces the limit value (the
> one which would cause the problem if it was 0) by the amount of bytes
> which have been read in the ByteChunk. This would translate to:
> Index: java/org/apache/catalina/connector/InputBuffer.java
> ===================================================================
> ---java/org/apache/catalina/connector/InputBuffer.java (revision 630535)
> +++java/org/apache/catalina/connector/InputBuffer.java (working copy)
> @@ -355,7 +355,7 @@
> }
>
> state = CHAR_STATE;
> - conv.convert(bb, cb, len);
> + conv.convert(bb, cb, bb.getLength());
> bb.setOffset(bb.getEnd());
>
> return cb.getLength();
>
> The problem is that I sort of like the idea of the limit (it's related
> to the space left in the char buffer for adding the parsed chars). So
> maybe trying to respect that with a decent fallback could work (didn't
> try it):
> Index: java/org/apache/catalina/connector/InputBuffer.java
> ===================================================================
> ---java/org/apache/catalina/connector/InputBuffer.java (revision 630535)
> +++java/org/apache/catalina/connector/InputBuffer.java (working copy)
> @@ -355,7 +355,7 @@
> }
>
> state = CHAR_STATE;
> - conv.convert(bb, cb, len);
> + conv.convert(bb, cb, (len == 0) ? bb.getLength() : len);
> bb.setOffset(bb.getEnd());
>
> return cb.getLength();
>
so now we're trying to "fix the fix". why is that? I do NOT claim that
"my shit don't stink" but lets face it, the broken patch came with such
claim. and now were arguing that it's "nicer".
sorry for being blunt, but lets cut to the chase here, what is it that
you are really trying to say and maybe we can focus the technical
discussion on that, instead of running around in circles.
Obviously the whole byte/char-chunk code is very sensitive, since
several components keep their own offset/end/limit flags to the same
byte[], which makes it somewhat of a difficult thing to read into. and
that's why I managed to come up with a patch that didn't upset that eco
system of the old code.
Filip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit:
r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Remy Maucherat <re...@apache.org>.
On Wed, 2008-02-27 at 17:08 -0700, Filip Hanik - Dev Lists wrote:
> Remy Maucherat wrote:
> > This makes me think there's a straight issue in CharChunk (I suppose the
> > problem is that realReadChars gets called with len = 0; if it handled
> > that scenario, things should be fine).
> >
> the old char chunk worked just fine, so why not stick with it. the error
> above is a result of B2CConverter and InputBuffer being modified
Ok, but the new code looks a bit nicer, so I would prefer keeping it if
possible. From what I can see, your code replaces the limit value (the
one which would cause the problem if it was 0) by the amount of bytes
which have been read in the ByteChunk. This would translate to:
Index: java/org/apache/catalina/connector/InputBuffer.java
===================================================================
---java/org/apache/catalina/connector/InputBuffer.java (revision 630535)
+++java/org/apache/catalina/connector/InputBuffer.java (working copy)
@@ -355,7 +355,7 @@
}
state = CHAR_STATE;
- conv.convert(bb, cb, len);
+ conv.convert(bb, cb, bb.getLength());
bb.setOffset(bb.getEnd());
return cb.getLength();
The problem is that I sort of like the idea of the limit (it's related
to the space left in the char buffer for adding the parsed chars). So
maybe trying to respect that with a decent fallback could work (didn't
try it):
Index: java/org/apache/catalina/connector/InputBuffer.java
===================================================================
---java/org/apache/catalina/connector/InputBuffer.java (revision 630535)
+++java/org/apache/catalina/connector/InputBuffer.java (working copy)
@@ -355,7 +355,7 @@
}
state = CHAR_STATE;
- conv.convert(bb, cb, len);
+ conv.convert(bb, cb, (len == 0) ? bb.getLength() : len);
bb.setOffset(bb.getEnd());
return cb.getLength();
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
Remy Maucherat wrote:
> On Wed, 2008-02-27 at 16:04 -0700, Filip Hanik - Dev Lists wrote:
>
>> Remy Maucherat wrote:
>>
>>>> unfortunately, the "elegant" solution caused a regression :(
>>>> http://issues.apache.org/bugzilla/show_bug.cgi?id=44494
>>>> I tested this with the inelegant (original proposed) patch and it
>>>> worked
>>>> fine, so I'm gonna fix this in trunk and propose the patch to 6.0
>>>>
>>>>
>>> Ok, and there's no time to think ? A patch must be applied within
>>>
>> the
>>
>>> next 5 seconds ? (probably because it was not your patch, I
>>>
>> assume :) )
>>
>>>
>>>
>> and lets cut the crap with the 5 second bs, I worked on this last
>> night,
>> and then again this morning when I saw how complicated the new patch
>> had
>> gotten and there was a very easy fix.
>>
>
> I'm looking your stacktrace which is supposedly:
> SEVERE: Servlet.service() for servlet jsp threw exception
> java.lang.ArrayIndexOutOfBoundsException: 8192
> at
> org.apache.tomcat.util.buf.CharChunk.substract(CharChunk.java:388)
> at
> org.apache.catalina.connector.InputBuffer.read(InputBuffer.java:368)
> at
> org.apache.catalina.connector.CoyoteReader.read(CoyoteReader.java:93)
>
> This makes me think there's a straight issue in CharChunk (I suppose the
> problem is that realReadChars gets called with len = 0; if it handled
> that scenario, things should be fine).
>
the old char chunk worked just fine, so why not stick with it. the error
above is a result of B2CConverter and InputBuffer being modified
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit: r568307
- /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Remy Maucherat <re...@apache.org>.
On Wed, 2008-02-27 at 16:04 -0700, Filip Hanik - Dev Lists wrote:
> Remy Maucherat wrote:
> >> unfortunately, the "elegant" solution caused a regression :(
> >> http://issues.apache.org/bugzilla/show_bug.cgi?id=44494
> >> I tested this with the inelegant (original proposed) patch and it
> >> worked
> >> fine, so I'm gonna fix this in trunk and propose the patch to 6.0
> >>
> >
> > Ok, and there's no time to think ? A patch must be applied within
> the
> > next 5 seconds ? (probably because it was not your patch, I
> assume :) )
> >
> and lets cut the crap with the 5 second bs, I worked on this last
> night,
> and then again this morning when I saw how complicated the new patch
> had
> gotten and there was a very easy fix.
I'm looking your stacktrace which is supposedly:
SEVERE: Servlet.service() for servlet jsp threw exception
java.lang.ArrayIndexOutOfBoundsException: 8192
at
org.apache.tomcat.util.buf.CharChunk.substract(CharChunk.java:388)
at
org.apache.catalina.connector.InputBuffer.read(InputBuffer.java:368)
at
org.apache.catalina.connector.CoyoteReader.read(CoyoteReader.java:93)
This makes me think there's a straight issue in CharChunk (I suppose the
problem is that realReadChars gets called with len = 0; if it handled
that scenario, things should be fine).
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
Remy Maucherat wrote:
> On Wed, 2008-02-27 at 12:51 -0700, Filip Hanik - Dev Lists wrote:
>
>> Bill Barker wrote:
>>
>>> "Remy Maucherat" <re...@apache.org> wrote in message
>>> news:46CB7F51.1030201@apache.org...
>>>
>>>
>>>> Filip Hanik - Dev Lists wrote:
>>>>
>>>>
>>>>> Test Case and 5.5.x patch can be found here.
>>>>> http://people.apache.org/~fhanik/tomcat/b2c/
>>>>>
>>>>> This is what is happening
>>>>>
>>>>> int cnt=conv.read( result, 0, BUFFER_SIZE );
>>>>> is called with a "while (true)" statement,
>>>>>
>>>>> When the IntermediateInputStream.read returns -1, the above
>>>>>
>> statement
>>
>>>>> returns cnt==1.
>>>>> So to avoid calling conv.read, we must check to see if we have
>>>>>
>> more bytes
>>
>>>>> to read by implementing the available() method, to avoid the
>>>>>
>> inputstream
>>
>>>>> ever returning -1.
>>>>>
>>>>>
>>>> It's possible, but I have a hard time understanding the issue.
>>>>
>>>>
>>>>
>>> The issue is that InputStreamReader reads 8192 bytes from
>>> IntermediateInputStream on the first go. It then translates them
>>>
>> into 2734
>>
>>> chars, but thinks that the last few bytes represent an incomplete
>>>
>> char, so
>>
>>> holds onto them. On the next call, IntermediateInputStream returns
>>>
>> -1, so
>>
>>> InputStreamReader outputs the last char as best it can (resulting
>>>
>> in
>>
>>> returning 1). Then the IntermediateInputStream buffer is reset, and
>>>
>> it can
>>
>>> continue on reading (but from the wrong position, resulting in
>>>
>> corruption).
>>
>>> Filip's patch is inelegant (better would be to use the ByteChunk
>>>
>> sink), but
>>
>>> other than my looking for a better way to do it, I can't come up
>>>
>> with the
>>
>>> required technical reason to porting the base of it to 5.5 (of
>>>
>> course, I
>>
>>> could care less what he does in his sandbox :).
>>>
>>>
>> unfortunately, the "elegant" solution caused a regression :(
>> http://issues.apache.org/bugzilla/show_bug.cgi?id=44494
>> I tested this with the inelegant (original proposed) patch and it
>> worked
>> fine, so I'm gonna fix this in trunk and propose the patch to 6.0
>>
>
> Ok, and there's no time to think ? A patch must be applied within the
> next 5 seconds ? (probably because it was not your patch, I assume :) )
>
and lets cut the crap with the 5 second bs, I worked on this last night,
and then again this morning when I saw how complicated the new patch had
gotten and there was a very easy fix.
> Rémy
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Filip Hanik - Dev Lists <de...@hanik.com>.
Remy Maucherat wrote:
> On Wed, 2008-02-27 at 12:51 -0700, Filip Hanik - Dev Lists wrote:
>
>> Bill Barker wrote:
>>
>>> "Remy Maucherat" <re...@apache.org> wrote in message
>>> news:46CB7F51.1030201@apache.org...
>>>
>>>
>>>> Filip Hanik - Dev Lists wrote:
>>>>
>>>>
>>>>> Test Case and 5.5.x patch can be found here.
>>>>> http://people.apache.org/~fhanik/tomcat/b2c/
>>>>>
>>>>> This is what is happening
>>>>>
>>>>> int cnt=conv.read( result, 0, BUFFER_SIZE );
>>>>> is called with a "while (true)" statement,
>>>>>
>>>>> When the IntermediateInputStream.read returns -1, the above
>>>>>
>> statement
>>
>>>>> returns cnt==1.
>>>>> So to avoid calling conv.read, we must check to see if we have
>>>>>
>> more bytes
>>
>>>>> to read by implementing the available() method, to avoid the
>>>>>
>> inputstream
>>
>>>>> ever returning -1.
>>>>>
>>>>>
>>>> It's possible, but I have a hard time understanding the issue.
>>>>
>>>>
>>>>
>>> The issue is that InputStreamReader reads 8192 bytes from
>>> IntermediateInputStream on the first go. It then translates them
>>>
>> into 2734
>>
>>> chars, but thinks that the last few bytes represent an incomplete
>>>
>> char, so
>>
>>> holds onto them. On the next call, IntermediateInputStream returns
>>>
>> -1, so
>>
>>> InputStreamReader outputs the last char as best it can (resulting
>>>
>> in
>>
>>> returning 1). Then the IntermediateInputStream buffer is reset, and
>>>
>> it can
>>
>>> continue on reading (but from the wrong position, resulting in
>>>
>> corruption).
>>
>>> Filip's patch is inelegant (better would be to use the ByteChunk
>>>
>> sink), but
>>
>>> other than my looking for a better way to do it, I can't come up
>>>
>> with the
>>
>>> required technical reason to porting the base of it to 5.5 (of
>>>
>> course, I
>>
>>> could care less what he does in his sandbox :).
>>>
>>>
>> unfortunately, the "elegant" solution caused a regression :(
>> http://issues.apache.org/bugzilla/show_bug.cgi?id=44494
>> I tested this with the inelegant (original proposed) patch and it
>> worked
>> fine, so I'm gonna fix this in trunk and propose the patch to 6.0
>>
>
> Ok, and there's no time to think ? A patch must be applied within the
> next 5 seconds ? (probably because it was not your patch, I assume :) )
>
the original patch was 4 lines of code change, that was considered
inelegant (as a matter of fact, it was dissed without even being looked
at), instead we made a more complex patch that caused regression. I
simply suggest that the simpler patch is easier to work with, and that
we make that available to the folks that want to patch their own version.
and available it is.
Filip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Bug in B2C converter WAS: svn commit: r568307 -
/tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Posted by Remy Maucherat <re...@apache.org>.
On Wed, 2008-02-27 at 12:51 -0700, Filip Hanik - Dev Lists wrote:
> Bill Barker wrote:
> > "Remy Maucherat" <re...@apache.org> wrote in message
> > news:46CB7F51.1030201@apache.org...
> >
> >> Filip Hanik - Dev Lists wrote:
> >>
> >>> Test Case and 5.5.x patch can be found here.
> >>> http://people.apache.org/~fhanik/tomcat/b2c/
> >>>
> >>> This is what is happening
> >>>
> >>> int cnt=conv.read( result, 0, BUFFER_SIZE );
> >>> is called with a "while (true)" statement,
> >>>
> >>> When the IntermediateInputStream.read returns -1, the above
> statement
> >>> returns cnt==1.
> >>> So to avoid calling conv.read, we must check to see if we have
> more bytes
> >>> to read by implementing the available() method, to avoid the
> inputstream
> >>> ever returning -1.
> >>>
> >> It's possible, but I have a hard time understanding the issue.
> >>
> >>
> >
> > The issue is that InputStreamReader reads 8192 bytes from
> > IntermediateInputStream on the first go. It then translates them
> into 2734
> > chars, but thinks that the last few bytes represent an incomplete
> char, so
> > holds onto them. On the next call, IntermediateInputStream returns
> -1, so
> > InputStreamReader outputs the last char as best it can (resulting
> in
> > returning 1). Then the IntermediateInputStream buffer is reset, and
> it can
> > continue on reading (but from the wrong position, resulting in
> corruption).
> >
> > Filip's patch is inelegant (better would be to use the ByteChunk
> sink), but
> > other than my looking for a better way to do it, I can't come up
> with the
> > required technical reason to porting the base of it to 5.5 (of
> course, I
> > could care less what he does in his sandbox :).
> >
> unfortunately, the "elegant" solution caused a regression :(
> http://issues.apache.org/bugzilla/show_bug.cgi?id=44494
> I tested this with the inelegant (original proposed) patch and it
> worked
> fine, so I'm gonna fix this in trunk and propose the patch to 6.0
Ok, and there's no time to think ? A patch must be applied within the
next 5 seconds ? (probably because it was not your patch, I assume :) )
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org