Re: [tomcat] branch main updated: Simplify - identified by SpotBugs

2021-06-24 Thread Christopher Schultz 

Mark,

On 6/24/21 03:59, Mark Thomas wrote:

On 23/06/2021 20:06, Christopher Schultz wrote:

Mark,

On 6/23/21 13:19, ma...@apache.org wrote:

This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/main by this push:
  new b9bd126  Simplify - identified by SpotBugs
b9bd126 is described below

commit b9bd12608d3a14ed036a1602f39b148d91fb5489
Author: Mark Thomas 
AuthorDate: Wed Jun 23 18:18:37 2021 +0100

 Simplify - identified by SpotBugs > ---
  java/org/apache/jasper/runtime/PageContextImpl.java | 17 
+

  1 file changed, 1 insertion(+), 16 deletions(-)


Will this not end up flushing buffers potentially needlessly early, 
converting responses to Chunked and preventing errors from being 
reported by committing the response?


No. This flushes the JSPWriterImpl. That flushes any data buffered in 
the JSPWriterImpl to the Writer obtained from the HttpServletResponse 
but that Writer is not then flushed.


Great, thanks for the explanation.

I realize that THIS patch doesn't do that, but it looks like the 
history of the file (c.f. the "Old code" comment) has two different 
behaviors depending upon whether the file is being included.


The "Old code" has been commented out for more than 15 years. You'd need 
to go back to the 5.5.x svn archives to understand what was going on here.


Glad to see it go, then.

Thanks,
-chris

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: [tomcat] branch main updated: Simplify - identified by SpotBugs

2021-06-24 Thread Mark Thomas 

On 23/06/2021 20:06, Christopher Schultz wrote:

Mark,

On 6/23/21 13:19, ma...@apache.org wrote:

This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/main by this push:
  new b9bd126  Simplify - identified by SpotBugs
b9bd126 is described below

commit b9bd12608d3a14ed036a1602f39b148d91fb5489
Author: Mark Thomas 
AuthorDate: Wed Jun 23 18:18:37 2021 +0100

 Simplify - identified by SpotBugs > ---
  java/org/apache/jasper/runtime/PageContextImpl.java | 17 
+

  1 file changed, 1 insertion(+), 16 deletions(-)


Will this not end up flushing buffers potentially needlessly early, 
converting responses to Chunked and preventing errors from being 
reported by committing the response?


No. This flushes the JSPWriterImpl. That flushes any data buffered in 
the JSPWriterImpl to the Writer obtained from the HttpServletResponse 
but that Writer is not then flushed.


I realize that THIS patch doesn't do that, but it looks like the history 
of the file (c.f. the "Old code" comment) has two different behaviors 
depending upon whether the file is being included.


The "Old code" has been commented out for more than 15 years. You'd need 
to go back to the 5.5.x svn archives to understand what was going on here.


Mark




-chris

diff --git a/java/org/apache/jasper/runtime/PageContextImpl.java 
b/java/org/apache/jasper/runtime/PageContextImpl.java

index f732273..1477013 100644
--- a/java/org/apache/jasper/runtime/PageContextImpl.java
+++ b/java/org/apache/jasper/runtime/PageContextImpl.java
@@ -97,8 +97,6 @@ public class PageContextImpl extends PageContext {
  private transient ELContextImpl elContext;
-    private boolean isIncluded;
-
  // initial output stream
  private transient JspWriter out;
@@ -170,26 +168,13 @@ public class PageContextImpl extends PageContext {
  setAttribute(CONFIG, config);
  setAttribute(PAGECONTEXT, this);
  setAttribute(APPLICATION, context);
-
-    isIncluded = request.getAttribute(
-    RequestDispatcher.INCLUDE_SERVLET_PATH) != null;
  }
  @Override
  public void release() {
  out = baseOut;
  try {
-    if (isIncluded) {
-    ((JspWriterImpl) out).flushBuffer();
-    // push it into the including jspWriter
-    } else {
-    // 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();
-    }
+    ((JspWriterImpl) out).flushBuffer();
  } catch (IOException ex) {
  IllegalStateException ise = new 
IllegalStateException(Localizer.getMessage("jsp.error.flush"), ex);

  throw ise;

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org




-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: [tomcat] branch main updated: Simplify - identified by SpotBugs

2021-06-23 Thread Christopher Schultz 

Mark,

On 6/23/21 13:19, ma...@apache.org wrote:

This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/main by this push:
  new b9bd126  Simplify - identified by SpotBugs
b9bd126 is described below

commit b9bd12608d3a14ed036a1602f39b148d91fb5489
Author: Mark Thomas 
AuthorDate: Wed Jun 23 18:18:37 2021 +0100

 Simplify - identified by SpotBugs > ---
  java/org/apache/jasper/runtime/PageContextImpl.java | 17 +
  1 file changed, 1 insertion(+), 16 deletions(-)


Will this not end up flushing buffers potentially needlessly early, 
converting responses to Chunked and preventing errors from being 
reported by committing the response?


I realize that THIS patch doesn't do that, but it looks like the history 
of the file (c.f. the "Old code" comment) has two different behaviors 
depending upon whether the file is being included.


-chris


diff --git a/java/org/apache/jasper/runtime/PageContextImpl.java 
b/java/org/apache/jasper/runtime/PageContextImpl.java
index f732273..1477013 100644
--- a/java/org/apache/jasper/runtime/PageContextImpl.java
+++ b/java/org/apache/jasper/runtime/PageContextImpl.java
@@ -97,8 +97,6 @@ public class PageContextImpl extends PageContext {
  
  private transient ELContextImpl elContext;
  
-private boolean isIncluded;

-
  
  // initial output stream

  private transient JspWriter out;
@@ -170,26 +168,13 @@ public class PageContextImpl extends PageContext {
  setAttribute(CONFIG, config);
  setAttribute(PAGECONTEXT, this);
  setAttribute(APPLICATION, context);
-
-isIncluded = request.getAttribute(
-RequestDispatcher.INCLUDE_SERVLET_PATH) != null;
  }
  
  @Override

  public void release() {
  out = baseOut;
  try {
-if (isIncluded) {
-((JspWriterImpl) out).flushBuffer();
-// push it into the including jspWriter
-} else {
-// 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();
-}
+((JspWriterImpl) out).flushBuffer();
  } catch (IOException ex) {
  IllegalStateException ise = new 
IllegalStateException(Localizer.getMessage("jsp.error.flush"), ex);
  throw ise;

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

[tomcat] branch main updated: Simplify - identified by SpotBugs

2021-06-23 Thread markt 
This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/main by this push:
 new b9bd126  Simplify - identified by SpotBugs
b9bd126 is described below

commit b9bd12608d3a14ed036a1602f39b148d91fb5489
Author: Mark Thomas 
AuthorDate: Wed Jun 23 18:18:37 2021 +0100

Simplify - identified by SpotBugs
---
 java/org/apache/jasper/runtime/PageContextImpl.java | 17 +
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/java/org/apache/jasper/runtime/PageContextImpl.java 
b/java/org/apache/jasper/runtime/PageContextImpl.java
index f732273..1477013 100644
--- a/java/org/apache/jasper/runtime/PageContextImpl.java
+++ b/java/org/apache/jasper/runtime/PageContextImpl.java
@@ -97,8 +97,6 @@ public class PageContextImpl extends PageContext {
 
 private transient ELContextImpl elContext;
 
-private boolean isIncluded;
-
 
 // initial output stream
 private transient JspWriter out;
@@ -170,26 +168,13 @@ public class PageContextImpl extends PageContext {
 setAttribute(CONFIG, config);
 setAttribute(PAGECONTEXT, this);
 setAttribute(APPLICATION, context);
-
-isIncluded = request.getAttribute(
-RequestDispatcher.INCLUDE_SERVLET_PATH) != null;
 }
 
 @Override
 public void release() {
 out = baseOut;
 try {
-if (isIncluded) {
-((JspWriterImpl) out).flushBuffer();
-// push it into the including jspWriter
-} else {
-// 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();
-}
+((JspWriterImpl) out).flushBuffer();
 } catch (IOException ex) {
 IllegalStateException ise = new 
IllegalStateException(Localizer.getMessage("jsp.error.flush"), ex);
 throw ise;

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org