Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix meaningless close #445

Merged
merged 1 commit into from
Nov 18, 2020
Merged

Fix meaningless close #445

merged 1 commit into from
Nov 18, 2020

Conversation

lcc
Copy link
Contributor

@lcc lcc commented Nov 12, 2020

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.

@lukaszlenart
Copy link
Member

Tests fail

@lcc
Copy link
Contributor Author

lcc commented Nov 12, 2020 •
edited

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 in the maven surefire plugin, in order to "fix" this one must update to a surefire version to one that already corrected it, 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.

@lukaszlenart
Copy link
Member

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

JCgH4164838Gh792C124B5
JCgH4164838Gh792C124B5 suggested changes Nov 14, 2020
View reviewed changes
Copy link
Contributor

@JCgH4164838Gh792C124B5 JCgH4164838Gh792C124B5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Lucas.

I think that the CharArrayWriter close() changes are OK, being no-ops as you pointed out.
The change to StrutsSpringPortletMockObjectsTest should probably be reverted, though, since it causes the compilation of your PR to fail.

Thanks,
James.

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same goes for here.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here.

plugins/portlet-mocks/src/test/java/org/apache/struts2/StrutsSpringPortletMockObjectsTest.java Outdated
@@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, the PR has now been updated!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.003%) to 49.776% when pulling 0339ff4 on lcc:fix-meaningless-close into f1dccc4 on apache:master.

lukaszlenart
lukaszlenart approved these changes Nov 18, 2020
View reviewed changes
Copy link
Member

@lukaszlenart lukaszlenart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@lukaszlenart lukaszlenart merged commit 8da6a53 into apache:master Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JCgH4164838Gh792C124B5 JCgH4164838Gh792C124B5 JCgH4164838Gh792C124B5 requested changes

@lukaszlenart lukaszlenart lukaszlenart approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@lcc @lukaszlenart @coveralls @JCgH4164838Gh792C124B5