Re: costin: fix reverted

[Remy Maucherat]

Costin Manolache wrote:


Remy Maucherat wrote:


>>Can you point me to the code doing this extra processing ?
>>
>>I'm a bit confused:
>>- for jsps ( with flush-in-release ) that just can't work, since the
>>flush is already done by the jsp page.
>>- the comment ( or the message ) in forward is probably wrong.
>>- my bigger question is when is the response not a facade ? My
>>understanding
>>is that facades are used all the time, so the second part of the if will
>>never happen
>
>That code was already there before the facades. I don't think it is used
>anymore, but would leave it anyway in 4.1.x (it could be removed in 5).


I already reverted the patch in 4.1.x - it's too dangerous for the
stable tree.

So you are saying that forward() used to do a flush, but it was
changed when facades where added to just set the flag - to improve
the error handling. Sounds consistent with the behavior we have now.


Yes, I think the error handling had problems before. Then I added some 
logic to the facades (bad) to fix that, and left the old code in the 
process.


Could you add a little comment ( or at least remove the debug message
that says 'flushing' ) ? The new behavior ( no real flush ) seems
correct.


Ok, I will.

Remy


--
To unsubscribe, e-mail:   
For additional commands, e-mail: 

Re: costin: fix reverted

[Costin Manolache]

I just tested - adding 'removeAttribute' in PageContextImpl after
the error page solves the problem.

The problem was pretty simple:
 - exception happens 
 - jsp error page called and executed
 - normal servlet exception handling is now processing the exception
 - output is reset by the error handler.

I'm pretty sure cleaning the exception after the jsp error page
is the right thing to do. I don't know how will this deal with
the case where an exception happens in the error page - but 
I can add code to verify the exception is the same ( i.e. remove
only if the throwable is the same )

Costin

Costin Manolache wrote:

> Remy Maucherat wrote:
> 
>>
>>> Can you point me to the code doing this extra processing ?
>>>
>>> I'm a bit confused:
>>> - for jsps ( with flush-in-release ) that just can't work, since the
>>> flush is already done by the jsp page.
>>> - the comment ( or the message ) in forward is probably wrong.
>>> - my bigger question is when is the response not a facade ? My
>>> understanding
>>> is that facades are used all the time, so the second part of the if will
>>> never happen
>> 
>> That code was already there before the facades. I don't think it is used
>> anymore, but would leave it anyway in 4.1.x (it could be removed in 5).
> 
> I already reverted the patch in 4.1.x - it's too dangerous for the
> stable tree.
> 
> So you are saying that forward() used to do a flush, but it was
> changed when facades where added to just set the flag - to improve
> the error handling. Sounds consistent with the behavior we have now.
> 
> Could you add a little comment ( or at least remove the debug message
> that says 'flushing' ) ? The new behavior ( no real flush ) seems
> correct.
> 
> Costin
> 
>>> I do see your point - if forward() flushes then the extra error
>>> processing is broken. That's another argument to not do the flush() in
>>> release() :-) And it does explain who is generating the stack trace.
>> 
>> The error page and status report processing is in
>> ErrorReport/DispatcherValve. That's where the stack trace is output
>> unless the response has been committed (by flushing usually).
> 
> 
> 
> 
>> 
>>>
>>> I have a feeling we just need to clear the error flags after the jsp
>>> error page - I'm pretty sure the jsp error page was executed and the
>>> data is in the out buffer. Somehow the servlet error processing kicks in
>>> and overrides the jsp error page.
>> 
>> Yes, that's likely the right explanation.
>> 
>> Remy




--
To unsubscribe, e-mail:   
For additional commands, e-mail: 

[possible bug] -- Re: costin: fix reverted

[peter lin]

On a related note, i just discovered some behavior that may be a bug. 
In my case, I have a request filter that buffers the output, so that I
can gzip it and time the internal time.

when a page hits an exception, it correctly forwards to my error page. 
but what ends up happening is the buffer isn't flushed, therefore the
output has unwanted data.  According to the spec, reset is supposed to
be called on ServletOutputStream, but when I print out debug messages, I
never see it call reset() or resetBuffer() in my output wrapper. It's
unclear to me if this part of the spec applies to requests that are
forwarded.

the relevant section from the spec is below.

5.1
--
The reset method clears data in the buffer when the response is not
committed. Headers and status codes set by the servlet prior to the
reset call must
be cleared as well. The resetBuffer method clears content in the buffer
if the
response is not committed without clearing the headers and status code.


does someone know what the correct behavior should be?


peter lin


Costin Manolache wrote:
> 
> Remy Maucherat wrote:
> 
> >
> >> Can you point me to the code doing this extra processing ?
> >>
> >> I'm a bit confused:
> >> - for jsps ( with flush-in-release ) that just can't work, since the
> >> flush is already done by the jsp page.
> >> - the comment ( or the message ) in forward is probably wrong.
> >> - my bigger question is when is the response not a facade ? My
> >> understanding
> >> is that facades are used all the time, so the second part of the if will
> >> never happen
> >
> > That code was already there before the facades. I don't think it is used
> > anymore, but would leave it anyway in 4.1.x (it could be removed in 5).
> 
> I already reverted the patch in 4.1.x - it's too dangerous for the
> stable tree.
> 
> So you are saying that forward() used to do a flush, but it was
> changed when facades where added to just set the flag - to improve
> the error handling. Sounds consistent with the behavior we have now.
> 
> Could you add a little comment ( or at least remove the debug message
> that says 'flushing' ) ? The new behavior ( no real flush ) seems
> correct.
> 
> Costin
> 
> >> I do see your point - if forward() flushes then the extra error
> >> processing is broken. That's another argument to not do the flush() in
> >> release() :-) And it does explain who is generating the stack trace.
> >
> > The error page and status report processing is in
> > ErrorReport/DispatcherValve. That's where the stack trace is output
> > unless the response has been committed (by flushing usually).
> 
> >
> >>
> >> I have a feeling we just need to clear the error flags after the jsp
> >> error page - I'm pretty sure the jsp error page was executed and the data
> >> is in the out buffer. Somehow the servlet error processing kicks in and
> >> overrides the jsp error page.
> >
> > Yes, that's likely the right explanation.
> >
> > Remy
> 
> --
> To unsubscribe, e-mail:   
> For additional commands, e-mail: 

--
To unsubscribe, e-mail:   
For additional commands, e-mail: 

Re: costin: fix reverted

[Costin Manolache]

Remy Maucherat wrote:

>
>> Can you point me to the code doing this extra processing ?
>>
>> I'm a bit confused:
>> - for jsps ( with flush-in-release ) that just can't work, since the
>> flush is already done by the jsp page.
>> - the comment ( or the message ) in forward is probably wrong.
>> - my bigger question is when is the response not a facade ? My
>> understanding
>> is that facades are used all the time, so the second part of the if will
>> never happen
> 
> That code was already there before the facades. I don't think it is used
> anymore, but would leave it anyway in 4.1.x (it could be removed in 5).

I already reverted the patch in 4.1.x - it's too dangerous for the 
stable tree.

So you are saying that forward() used to do a flush, but it was
changed when facades where added to just set the flag - to improve
the error handling. Sounds consistent with the behavior we have now.

Could you add a little comment ( or at least remove the debug message
that says 'flushing' ) ? The new behavior ( no real flush ) seems 
correct.

Costin

>> I do see your point - if forward() flushes then the extra error
>> processing is broken. That's another argument to not do the flush() in
>> release() :-) And it does explain who is generating the stack trace.
> 
> The error page and status report processing is in
> ErrorReport/DispatcherValve. That's where the stack trace is output
> unless the response has been committed (by flushing usually).




> 
>>
>> I have a feeling we just need to clear the error flags after the jsp
>> error page - I'm pretty sure the jsp error page was executed and the data
>> is in the out buffer. Somehow the servlet error processing kicks in and
>> overrides the jsp error page.
> 
> Yes, that's likely the right explanation.
> 
> Remy




--
To unsubscribe, e-mail:   
For additional commands, e-mail: 

Re: costin: fix reverted

[Remy Maucherat]

Costin Manolache wrote:


Remy Maucherat wrote:


>>In any case, I expect this to have caused some weird behavior
>>for normal forward - since forward doesn't seem to really flush/close
>>as it was supposed to do ( unless response is not facade - does
>>this case ever happen ? ). A bit strange no other test detected that,
>>normal servlets don't have flush/close that the jsp page had.
>
>No, that wouldn't work.
>forward does a fake flush/close, because some further error page
>processing may occur (based on the status code, for example).
>
>I think we'll have to do the commit in the JSP error page itself (and
>call close right away in the case, rather than flush). I hope it's 
doable.


Can you point me to the code doing this extra processing ?

I'm a bit confused:
- for jsps ( with flush-in-release ) that just can't work, since the flush
is already done by the jsp page.
- the comment ( or the message ) in forward is probably wrong.
- my bigger question is when is the response not a facade ? My 
understanding
is that facades are used all the time, so the second part of the if will
never happen

That code was already there before the facades. I don't think it is used 
anymore, but would leave it anyway in 4.1.x (it could be removed in 5).



I do see your point - if forward() flushes then the extra error processing
is broken. That's another argument to not do the flush() in release() :-)
And it does explain who is generating the stack trace.


The error page and status report processing is in 
ErrorReport/DispatcherValve. That's where the stack trace is output 
unless the response has been committed (by flushing usually).


I have a feeling we just need to clear the error flags after the jsp error
page - I'm pretty sure the jsp error page was executed and the data is
in the out buffer. Somehow the servlet error processing kicks in and
overrides the jsp error page.


Yes, that's likely the right explanation.

Remy


--
To unsubscribe, e-mail:   
For additional commands, e-mail: 

Re: costin: fix reverted

[Costin Manolache]

Remy Maucherat wrote:

>> In any case, I expect this to have caused some weird behavior
>> for normal forward - since forward doesn't seem to really flush/close
>> as it was supposed to do ( unless response is not facade - does
>> this case ever happen ? ). A bit strange no other test detected that,
>> normal servlets don't have flush/close that the jsp page had.
> 
> No, that wouldn't work.
> forward does a fake flush/close, because some further error page
> processing may occur (based on the status code, for example).
> 
> I think we'll have to do the commit in the JSP error page itself (and
> call close right away in the case, rather than flush). I hope it's doable.

Can you point me to the code doing this extra processing ?

I'm a bit confused:
- for jsps ( with flush-in-release ) that just can't work, since the flush
is already done by the jsp page. 
- the comment ( or the message ) in forward is probably wrong. 
- my bigger question is when is the response not a facade ? My understanding
is that facades are used all the time, so the second part of the if will
never happen

I do see your point - if forward() flushes then the extra error processing
is broken. That's another argument to not do the flush() in release() :-)
And it does explain who is generating the stack trace.

I have a feeling we just need to clear the error flags after the jsp error
page - I'm pretty sure the jsp error page was executed and the data is
in the out buffer. Somehow the servlet error processing kicks in and
overrides the jsp error page.

Costin






--
To unsubscribe, e-mail:   
For additional commands, e-mail: 

Re: costin: fix reverted

[Remy Maucherat]

Remy Maucherat wrote:


Costin Manolache wrote:

> I'm getting closer - Remy or Kin-Man, I need your help.
> As I expected, the flush() was hiding some other behaviors.
>
> Right now I'm looking at ApplicationDispatcher.doForward().
>
> What happens is:
>  - error happens
>  - we forward to the error page
>  - error page executes, put the data in the out buffer
>  - BEFORE: the out buffer was commited
>  - now we just return from servlet to doForward()
>  - The "commiting and closing response" reached ( line 471 )
>  What happens here is response is a ResponseFacade, and finish
> is called. But the real flushing and closing doesn't actually
> happens.
>
> I changed the code to first flush/close, the call finish - and
> it now seems to work fine.
>
> But I'm not sure what else may be affected by this - or if
> it's safe to make the change ( well, for 5.0 it may be, but what
> about 4.1 ? )
>
> In any case, I expect this to have caused some weird behavior
> for normal forward - since forward doesn't seem to really flush/close
> as it was supposed to do ( unless response is not facade - does
> this case ever happen ? ). A bit strange no other test detected that,
> normal servlets don't have flush/close that the jsp page had.


No, that wouldn't work.
forward does a fake flush/close, because some further error page
processing may occur (based on the status code, for example).

I think we'll have to do the commit in the JSP error page itself (and
call close right away in the case, rather than flush). I hope it's doable.



Given the risk of this refactoring, I propose delaying it in 4.1.x until 
at least after the next stable release.

Remy


--
To unsubscribe, e-mail:   
For additional commands, e-mail: 

Re: costin: fix reverted

[Remy Maucherat]

Costin Manolache wrote:


I'm getting closer - Remy or Kin-Man, I need your help.
As I expected, the flush() was hiding some other behaviors.

Right now I'm looking at ApplicationDispatcher.doForward().

What happens is:
 - error happens
 - we forward to the error page
 - error page executes, put the data in the out buffer
 - BEFORE: the out buffer was commited
 - now we just return from servlet to doForward()
 - The "commiting and closing response" reached ( line 471 )
 What happens here is response is a ResponseFacade, and finish
is called. But the real flushing and closing doesn't actually
happens.

I changed the code to first flush/close, the call finish - and
it now seems to work fine.

But I'm not sure what else may be affected by this - or if
it's safe to make the change ( well, for 5.0 it may be, but what
about 4.1 ? )

In any case, I expect this to have caused some weird behavior
for normal forward - since forward doesn't seem to really flush/close
as it was supposed to do ( unless response is not facade - does
this case ever happen ? ). A bit strange no other test detected that,
normal servlets don't have flush/close that the jsp page had.


No, that wouldn't work.
forward does a fake flush/close, because some further error page 
processing may occur (based on the status code, for example).

I think we'll have to do the commit in the JSP error page itself (and 
call close right away in the case, rather than flush). I hope it's doable.

Remy


--
To unsubscribe, e-mail:   
For additional commands, e-mail: 

Re: costin: fix reverted

[Costin Manolache]

I'm getting closer - Remy or Kin-Man, I need your help.
As I expected, the flush() was hiding some other behaviors.

Right now I'm looking at ApplicationDispatcher.doForward().

What happens is:
 - error happens
 - we forward to the error page
 - error page executes, put the data in the out buffer
 - BEFORE: the out buffer was commited
 - now we just return from servlet to doForward()
 - The "commiting and closing response" reached ( line 471 )
 What happens here is response is a ResponseFacade, and finish
is called. But the real flushing and closing doesn't actually 
happens.

I changed the code to first flush/close, the call finish - and
it now seems to work fine.

But I'm not sure what else may be affected by this - or if 
it's safe to make the change ( well, for 5.0 it may be, but what
about 4.1 ? )

In any case, I expect this to have caused some weird behavior 
for normal forward - since forward doesn't seem to really flush/close
as it was supposed to do ( unless response is not facade - does
this case ever happen ? ). A bit strange no other test detected that,
normal servlets don't have flush/close that the jsp page had.


Costin



Costin Manolache wrote:

> I get more info on the issue. The error page worked before
> because the forward() that was used to redirect to the
> error page did commit the buffer.
> 
> Right now the control returns to the page with error, where
> the buffer seems to be overriden.
> 
> I'm still debugging it - if I can't find any simple fix I'll
> just roll back in the 4.1 tree, and continue to try for 5.0
> 
> Costin




--
To unsubscribe, e-mail:   
For additional commands, e-mail: 

Re: costin: fix reverted

[Costin Manolache]

I get more info on the issue. The error page worked before
because the forward() that was used to redirect to the 
error page did commit the buffer. 

Right now the control returns to the page with error, where
the buffer seems to be overriden. 

I'm still debugging it - if I can't find any simple fix I'll
just roll back in the 4.1 tree, and continue to try for 5.0

Costin



--
To unsubscribe, e-mail:   
For additional commands, e-mail: 

Re: costin: fix reverted

[Remy Maucherat]

Costin Manolache wrote:


I'll work on this ( I'm in the middle of something - as soon as I get
some time, this is on the top of my list ).



>>I reverted tc5 for my own convenience.  I didn't revert tomcat_4_branch
>>becuase I thought that you may want hack more there.  :-)


Yes, but if it brakes something... I'll revert it today - hopefully 
4.1.15
is not yet built.

I didn't plan to tag 4.1.15 right now. At least not before that flushing 
issue is resolved (ie, the flush is gone, unless it implies some major 
refactoring in Jasper).

Performance is the secondary issue here - if the spec doesn't define it
then it's a compliance bug. Side effects ( like flushing the out stream )
must be defined in the method documentation. 
Flushing/commiting/closing the
stream is too important.

Yes, I agree it could have some problematic side effects.

Remy


--
To unsubscribe, e-mail:   
For additional commands, e-mail: 

Re: costin: fix reverted

[Costin Manolache]

I'll work on this ( I'm in the middle of something - as soon as I get 
some time, this is on the top of my list ). 


>> I reverted tc5 for my own convenience.  I didn't revert tomcat_4_branch
>> becuase I thought that you may want hack more there.  :-)

Yes, but if it brakes something... I'll revert it today - hopefully 4.1.15 
is not yet built.

>> >I think the main question is if releasePageContext is required by the
>> >spec to flush() ( i.e. commit the message ) or not.
>> >
>>
>>
>> The spec is not specific about this.  So it is up to the
>> implementation. :-)
> 
> Good, then we *have to* avoid flushing. (better performance from the
> client side - Coyote makes chunking almost free already, so no gain on
> the server -, plus we comply better with the spirit of the specification).


Performance is the secondary issue here - if the spec doesn't define it
then it's a compliance bug. Side effects ( like flushing the out stream ) 
must be defined in the method documentation. Flushing/commiting/closing the 
stream is too important.

Kin-Man - if you have some quick way to get a confirmation from some
jsp spec people - it could save us some time :-)


> It seems like an unwanted Jasper limitation. We should fix it.

I'll do it ASAP. Sorry I couldn't yesterday.

Costin



--
To unsubscribe, e-mail:   
For additional commands, e-mail: 

Re: costin: fix reverted

[Remy Maucherat]

Kin-Man Chung wrote:


>>costin,
>>
>>This fix seems to break errorPage handling in JSP, causing the errorPage
>>example to fail, and a couple of JSP watchdog tests too.  I have 
reverted
>>your fix.
>>
>>I have not reverted the tomcat_4_branch.
>
>If it breaks something - tomcat_4_branch should be the first to 
revert :-)
>

I reverted tc5 for my own convenience.  I didn't revert tomcat_4_branch
becuase I thought that you may want hack more there.  :-)


>I think the main question is if releasePageContext is required by the
>spec to flush() ( i.e. commit the message ) or not.
>


The spec is not specific about this.  So it is up to the 
implementation. :-)

Good, then we *have to* avoid flushing. (better performance from the 
client side - Coyote makes chunking almost free already, so no gain on 
the server -, plus we comply better with the spirit of the specification).



>If it is required - then Content-Length just can't be set by the 
container
>for jsps ( which is not the end of the world :-).
>
>If the spec doesn't require that ( and my reading is that 
releasePageContext
>doc doesn't in any way imply this as a side effect - the flush() is a 
very
>different API ) - then I would say the tests are wrong.
>


It's not that the tests are wrong, but that your 'fix' renders any JSP
error page not work at all.  When an exception occurs, instead of 
displaying
the error page, it displays the stack trace of the exception!  I don't 
have
anything against not calling flush() in release, but you'll also need to
make sure that it doesn't break this.

It seems like an unwanted Jasper limitation. We should fix it.

BTW, I agree with Costin, the patch should be reverted first from 4.1.x. 
As for 5.0.x, it is not in alpha yet on the ASF side, so it would seem 
more productive if the J2EE patch were selecting a previous version of 
the patch rather than just reverting it, as it will have to be applied 
anyway.

Remy


--
To unsubscribe, e-mail:   
For additional commands, e-mail: 

Re: costin: fix reverted

[Kin-Man Chung]

> X-Trace: main.gmane.org 1036447809 25208 64.84.39.162 (4 Nov 2002 22:10:09 
GMT)
> Date: Mon, 04 Nov 2002 14:11:32 -0800
> From: Costin Manolache <[EMAIL PROTECTED]>
> Subject: Re: costin: fix reverted
> To: [EMAIL PROTECTED]
> X-Complaints-to: [EMAIL PROTECTED]
> NNTP-posting-date: Mon, 4 Nov 2002 22:10:09 + (UTC)
> X-Injected-Via-Gmane: http://gmane.org/
> NNTP-posting-host: 64.84.39.162
> 
> Kin-Man Chung wrote:
> 
> > costin,
> > 
> > This fix seems to break errorPage handling in JSP, causing the errorPage
> > example to fail, and a couple of JSP watchdog tests too.  I have reverted
> > your fix.
> > 
> > I have not reverted the tomcat_4_branch.
> 
> If it breaks something - tomcat_4_branch should be the first to revert :-)
> 
I reverted tc5 for my own convenience.  I didn't revert tomcat_4_branch
becuase I thought that you may want hack more there.  :-)

> I think the main question is if releasePageContext is required by the 
> spec to flush() ( i.e. commit the message ) or not. 
> 

The spec is not specific about this.  So it is up to the implementation. :-)

> If it is required - then Content-Length just can't be set by the container
> for jsps ( which is not the end of the world :-).
> 
> If the spec doesn't require that ( and my reading is that releasePageContext 
> doc doesn't in any way imply this as a side effect - the flush() is a very
> different API ) - then I would say the tests are wrong.
> 

It's not that the tests are wrong, but that your 'fix' renders any JSP
error page not work at all.  When an exception occurs, instead of displaying
the error page, it displays the stack trace of the exception!  I don't have
anything against not calling flush() in release, but you'll also need to
make sure that it doesn't break this.

> 
> 
> Costin
> 
> 
> > 
> >> Date: Thu, 24 Oct 2002 19:18:55 +
> >> From: [EMAIL PROTECTED]
> >> Subject: cvs commit:
> > jakarta-tomcat-jasper/jasper2/src/share/org/apache/jasper/runtime
> > PageContextImpl.java
> >> To: [EMAIL PROTECTED]
> >> 
> >> costin  2002/10/24 12:18:55
> >> 
> >>   Modified:jasper2/src/share/org/apache/jasper/runtime
> >> PageContextImpl.java
> >>   Log:
> >>   Change the 'flush' to just a 'flushBuffer'.
> >>   
> >>   This allows the container to deal with flushing the buffer (
> >>   wich is done automatically if the servlet doesn't explicitely
> >>   flush()/close() ). The container can attach the Content-Length
> >>   header which is usefull in many cases.
> >>   
> >>   Revision  ChangesPath
> >>   1.27  +11 -6
> > 
> 
jakarta-tomcat-jasper/jasper2/src/share/org/apache/jasper/runtime/PageContextImp
> > l.java
> >>   
> >>   Index: PageContextImpl.java
> >>   ===
> >>   RCS file:
> > 
> 
/home/cvs/jakarta-tomcat-jasper/jasper2/src/share/org/apache/jasper/runtime/Page
> > ContextImpl.java,v
> >>   retrieving revision 1.26
> >>   retrieving revision 1.27
> >>   diff -u -r1.26 -r1.27
> >>   --- PageContextImpl.java   4 Oct 2002 19:21:44 -   1.26
> >>   +++ PageContextImpl.java   24 Oct 2002 19:18:55 -  1.27
> >>   @@ -162,7 +162,7 @@
> >>this.bufferSize   = bufferSize;
> >>this.autoFlush= autoFlush;
> >>this.request  = request;
> >>   -  this.response = response;
> >>   +  this.response = response;
> >>
> >>// setup session (if required)
> >>if (request instanceof HttpServletRequest && needsSession)
> >>   @@ -209,7 +209,12 @@
> >>((JspWriterImpl)out).flushBuffer();
> >>// push it into the including jspWriter
> >>} else {
> >>   -  out.flush();
> >>   +// Old code:
> >>   +  //out.flush();
> >>   +// Do not flush the buffer even if we're not included
> >>   (i.e.
> >>   +// we are the main page. The servlet will flush it and
> > close
> >>   +// the stream.
> >>   +((JspWriterImpl)out).flushBuffer();
> >>}
> >>} catch (IOException ex) {
> >>loghelper.log("Internal error flushing the buffer in release()");
> >>   @@ -226,7 +231,7 @@
> >>depth = -1;
> >>baseOut.recycle();
> >>session  = null;
> >>   -
> >>   +
> >>attributes.clear();
> >>}
> >>
> >>   
> >>   
> >>   
> >> 
> >> --
> >> To unsubscribe, e-mail:  
> >> <mailto:tomcat-dev-unsubscribe@;jakarta.apache.org> For additional
> >> commands, e-mail: <mailto:tomcat-dev-help@;jakarta.apache.org>
> >>
> 
> 
> 
> 
> --
> To unsubscribe, e-mail:   <mailto:tomcat-dev-unsubscribe@;jakarta.apache.org>
> For additional commands, e-mail: <mailto:tomcat-dev-help@;jakarta.apache.org>
> 


--
To unsubscribe, e-mail:   <mailto:tomcat-dev-unsubscribe@;jakarta.apache.org>
For additional commands, e-mail: <mailto:tomcat-dev-help@;jakarta.apache.org>

Re: costin: fix reverted

[Costin Manolache]

Kin-Man Chung wrote:

> costin,
> 
> This fix seems to break errorPage handling in JSP, causing the errorPage
> example to fail, and a couple of JSP watchdog tests too.  I have reverted
> your fix.
> 
> I have not reverted the tomcat_4_branch.

If it breaks something - tomcat_4_branch should be the first to revert :-)

I think the main question is if releasePageContext is required by the 
spec to flush() ( i.e. commit the message ) or not. 

If it is required - then Content-Length just can't be set by the container
for jsps ( which is not the end of the world :-).

If the spec doesn't require that ( and my reading is that releasePageContext 
doc doesn't in any way imply this as a side effect - the flush() is a very
different API ) - then I would say the tests are wrong.



Costin


> 
>> Date: Thu, 24 Oct 2002 19:18:55 +
>> From: [EMAIL PROTECTED]
>> Subject: cvs commit:
> jakarta-tomcat-jasper/jasper2/src/share/org/apache/jasper/runtime
> PageContextImpl.java
>> To: [EMAIL PROTECTED]
>> 
>> costin  2002/10/24 12:18:55
>> 
>>   Modified:jasper2/src/share/org/apache/jasper/runtime
>> PageContextImpl.java
>>   Log:
>>   Change the 'flush' to just a 'flushBuffer'.
>>   
>>   This allows the container to deal with flushing the buffer (
>>   wich is done automatically if the servlet doesn't explicitely
>>   flush()/close() ). The container can attach the Content-Length
>>   header which is usefull in many cases.
>>   
>>   Revision  ChangesPath
>>   1.27  +11 -6
> 
jakarta-tomcat-jasper/jasper2/src/share/org/apache/jasper/runtime/PageContextImp
> l.java
>>   
>>   Index: PageContextImpl.java
>>   ===
>>   RCS file:
> 
/home/cvs/jakarta-tomcat-jasper/jasper2/src/share/org/apache/jasper/runtime/Page
> ContextImpl.java,v
>>   retrieving revision 1.26
>>   retrieving revision 1.27
>>   diff -u -r1.26 -r1.27
>>   --- PageContextImpl.java   4 Oct 2002 19:21:44 -   1.26
>>   +++ PageContextImpl.java   24 Oct 2002 19:18:55 -  1.27
>>   @@ -162,7 +162,7 @@
>>this.bufferSize   = bufferSize;
>>this.autoFlush= autoFlush;
>>this.request  = request;
>>   -  this.response = response;
>>   +  this.response = response;
>>
>>// setup session (if required)
>>if (request instanceof HttpServletRequest && needsSession)
>>   @@ -209,7 +209,12 @@
>>((JspWriterImpl)out).flushBuffer();
>>// push it into the including jspWriter
>>} else {
>>   -  out.flush();
>>   +// Old code:
>>   +  //out.flush();
>>   +// Do not flush the buffer even if we're not included
>>   (i.e.
>>   +// we are the main page. The servlet will flush it and
> close
>>   +// the stream.
>>   +((JspWriterImpl)out).flushBuffer();
>>}
>>} catch (IOException ex) {
>>loghelper.log("Internal error flushing the buffer in release()");
>>   @@ -226,7 +231,7 @@
>>depth = -1;
>>baseOut.recycle();
>>session  = null;
>>   -
>>   +
>>attributes.clear();
>>}
>>
>>   
>>   
>>   
>> 
>> --
>> To unsubscribe, e-mail:  
>>  For additional
>> commands, e-mail: 
>>




--
To unsubscribe, e-mail:   
For additional commands, e-mail: 

costin: fix reverted

[Kin-Man Chung]

costin,

This fix seems to break errorPage handling in JSP, causing the errorPage
example to fail, and a couple of JSP watchdog tests too.  I have reverted
your fix.

I have not reverted the tomcat_4_branch.

> Date: Thu, 24 Oct 2002 19:18:55 +
> From: [EMAIL PROTECTED]
> Subject: cvs commit: 
jakarta-tomcat-jasper/jasper2/src/share/org/apache/jasper/runtime 
PageContextImpl.java
> To: [EMAIL PROTECTED]
> 
> costin  2002/10/24 12:18:55
> 
>   Modified:jasper2/src/share/org/apache/jasper/runtime
> PageContextImpl.java
>   Log:
>   Change the 'flush' to just a 'flushBuffer'.
>   
>   This allows the container to deal with flushing the buffer (
>   wich is done automatically if the servlet doesn't explicitely
>   flush()/close() ). The container can attach the Content-Length
>   header which is usefull in many cases.
>   
>   Revision  ChangesPath
>   1.27  +11 -6 
jakarta-tomcat-jasper/jasper2/src/share/org/apache/jasper/runtime/PageContextImp
l.java
>   
>   Index: PageContextImpl.java
>   ===
>   RCS file: 
/home/cvs/jakarta-tomcat-jasper/jasper2/src/share/org/apache/jasper/runtime/Page
ContextImpl.java,v
>   retrieving revision 1.26
>   retrieving revision 1.27
>   diff -u -r1.26 -r1.27
>   --- PageContextImpl.java4 Oct 2002 19:21:44 -   1.26
>   +++ PageContextImpl.java24 Oct 2002 19:18:55 -  1.27
>   @@ -162,7 +162,7 @@
>   this.bufferSize   = bufferSize;
>   this.autoFlush= autoFlush;
>   this.request  = request;
>   -   this.response = response;
>   +   this.response = response;
>
>   // setup session (if required)
>   if (request instanceof HttpServletRequest && needsSession)
>   @@ -209,7 +209,12 @@
>   ((JspWriterImpl)out).flushBuffer();
>   // push it into the including jspWriter
>   } else {
>   -   out.flush();
>   +// Old code:
>   +   //out.flush();
>   +// Do not flush the buffer even if we're not included (i.e.
>   +// we are the main page. The servlet will flush it and 
close
>   +// the stream.
>   +((JspWriterImpl)out).flushBuffer();
>   }
>   } catch (IOException ex) {
>   loghelper.log("Internal error flushing the buffer in release()");
>   @@ -226,7 +231,7 @@
>depth = -1;
>   baseOut.recycle();
>   session  = null;
>   -
>   + 
>   attributes.clear();
>}
>
>   
>   
>   
> 
> --
> To unsubscribe, e-mail:   
> For additional commands, e-mail: 
> 


--
To unsubscribe, e-mail:   
For additional commands, e-mail: