You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by GitBox <gi...@apache.org> on 2020/11/12 00:16:37 UTC

[GitHub] [struts] lcc opened a new pull request #445: Fix meaningless close

lcc opened a new pull request #445:
URL: https://github.com/apache/struts/pull/445


   Meaningless Close: In several classes, close(), specified in Closeable interface, has no effect and other methods in those classes can be called after close() without IOException.
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org

[GitHub] [struts] lukaszlenart commented on pull request #445: Fix meaningless close

Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #445:
URL: https://github.com/apache/struts/pull/445#issuecomment-726138589


   As far I see there is a compilation error
   ```
   [ERROR] COMPILATION ERROR : 
   [INFO] -------------------------------------------------------------
   [ERROR] /home/travis/build/apache/struts/plugins/portlet-mocks/src/test/java/org/apache/struts2/StrutsSpringPortletMockObjectsTest.java:[225,78] unreported exception java.io.IOException; must be caught or declared to be thrown
   ```
   
   https://travis-ci.org/github/apache/struts/jobs/743068291


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [struts] coveralls commented on pull request #445: Fix meaningless close

Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #445:
URL: https://github.com/apache/struts/pull/445#issuecomment-729325981


   
   [![Coverage Status](https://coveralls.io/builds/35045772/badge)](https://coveralls.io/builds/35045772)
   
   Coverage decreased (-0.003%) to 49.776% when pulling **0339ff4550a4975e6f293ce7fde77eeffbf9acb2 on lcc:fix-meaningless-close** into **f1dccc4610b9455a0e18c8f5d24bd9df945fbb0d on apache:master**.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [struts] lcc commented on pull request #445: Fix meaningless close

Posted by GitBox <gi...@apache.org>.
lcc commented on pull request #445:
URL: https://github.com/apache/struts/pull/445#issuecomment-726109984


   Hi @lukaszlenart thanks for the prompt response. The error that causes the pipeline to fail is "ClassNotFoundException org.apache.maven.surefire.booter.ForkedBooter" which also happens in the current master build. 
   
   This is a [well known error](https://issues.apache.org/jira/browse/SUREFIRE-1588) in the maven surefire plugin, in order to "fix" this one must update to a surefire version to one that corrected this, such as 3.0.0-M1.
   
   After updating it on my surefire-plugin, this issue ceases to exist and all tests pass.
   
   This PR was purely meant to fix unecessary uses of the close method, but if you want I can write a patch for the surefire-plugin.
   
   I didn't do it before because there may be some underlying reason to use it the way it's being used.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [struts] lcc commented on a change in pull request #445: Fix meaningless close

Posted by GitBox <gi...@apache.org>.
lcc commented on a change in pull request #445:
URL: https://github.com/apache/struts/pull/445#discussion_r525560074



##########
File path: plugins/portlet-mocks/src/test/java/org/apache/struts2/StrutsSpringPortletMockObjectsTest.java
##########
@@ -222,7 +222,8 @@ public void testMockClientDataRequest() {
         assertEquals("Content-type not as expected ?", TEST_CONTENT_TYPE, mockClientDataRequest.getContentType());
         mockClientDataRequest.setMethod(TEST_METHOD);
         assertEquals("Method not as expected ?", TEST_METHOD, mockClientDataRequest.getMethod());
-        try ( InputStream inputStream = mockClientDataRequest.getPortletInputStream() ) {
+	InputStream inputStream = mockClientDataRequest.getPortletInputStream();

Review comment:
       Thanks for the suggestion, the PR has now been updated!




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [struts] JCgH4164838Gh792C124B5 commented on a change in pull request #445: Fix meaningless close

Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on a change in pull request #445:
URL: https://github.com/apache/struts/pull/445#discussion_r523474534



##########
File path: plugins/embeddedjsp/src/main/java/org/apache/struts2/jasper/compiler/Parser.java
##########
@@ -324,7 +324,6 @@ private String parseScriptText(String tx) {
                 ++i;
             }
         }
-        cw.close();

Review comment:
       And here.

##########
File path: plugins/embeddedjsp/src/main/java/org/apache/struts2/jasper/compiler/JspReader.java
##########
@@ -595,7 +594,6 @@ private void pushFile(String file, String encoding,
             char buf[] = new char[1024];
             for (int i = 0 ; (i = reader.read(buf)) != -1 ;)
                 caw.write(buf, 0, i);
-            caw.close();

Review comment:
       Same goes for here.

##########
File path: plugins/embeddedjsp/src/main/java/org/apache/struts2/jasper/compiler/JspReader.java
##########
@@ -209,7 +209,6 @@ String getText(Mark start, Mark stop) throws JasperException {
         CharArrayWriter caw = new CharArrayWriter();
         while (!stop.equals(mark()))
             caw.write(nextChar());
-        caw.close();

Review comment:
       The API Docs and JDK source confirm that for `CharArrayWriter` instances, calls to `close()` are no-ops (no effect).
   So I think this should be a safe change.

##########
File path: plugins/portlet-mocks/src/test/java/org/apache/struts2/StrutsSpringPortletMockObjectsTest.java
##########
@@ -222,7 +222,8 @@ public void testMockClientDataRequest() {
         assertEquals("Content-type not as expected ?", TEST_CONTENT_TYPE, mockClientDataRequest.getContentType());
         mockClientDataRequest.setMethod(TEST_METHOD);
         assertEquals("Method not as expected ?", TEST_METHOD, mockClientDataRequest.getMethod());
-        try ( InputStream inputStream = mockClientDataRequest.getPortletInputStream() ) {
+	InputStream inputStream = mockClientDataRequest.getPortletInputStream();

Review comment:
       This change in the PR should probably be reverted.  The previous try-with-resources call is safer, and restoring that code should eliminate the `IOException` issue currently being reported during the compilation.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [struts] lukaszlenart commented on pull request #445: Fix meaningless close

Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #445:
URL: https://github.com/apache/struts/pull/445#issuecomment-725895359


   Tests fail


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [struts] lukaszlenart merged pull request #445: Fix meaningless close

Posted by GitBox <gi...@apache.org>.
lukaszlenart merged pull request #445:
URL: https://github.com/apache/struts/pull/445


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org