You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/01/30 13:22:00 UTC
[GitHub] [maven] garydgregory opened a new pull request #669: Use try-with-resources
garydgregory opened a new pull request #669:
URL: https://github.com/apache/maven/pull/669
Trivial: Make sure resources are closed even when an exception is thrown by using try-with-resources.
- [X] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
mvn clean package OK
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #669: Use try-with-resources
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #669:
URL: https://github.com/apache/maven/pull/669#discussion_r800095831
##########
File path: maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java
##########
@@ -216,9 +216,10 @@ private PluginDescriptor extractPluginDescriptor( Artifact pluginArtifact, Plugi
if ( pluginDescriptorEntry != null )
{
- InputStream is = pluginJar.getInputStream( pluginDescriptorEntry );
-
- pluginDescriptor = parsePluginDescriptor( is, plugin, pluginFile.getAbsolutePath() );
+ try ( InputStream is = pluginJar.getInputStream( pluginDescriptorEntry ) )
Review comment:
Agree here.
##########
File path: maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java
##########
@@ -216,9 +216,10 @@ private PluginDescriptor extractPluginDescriptor( Artifact pluginArtifact, Plugi
if ( pluginDescriptorEntry != null )
{
- InputStream is = pluginJar.getInputStream( pluginDescriptorEntry );
-
- pluginDescriptor = parsePluginDescriptor( is, plugin, pluginFile.getAbsolutePath() );
+ try ( InputStream is = pluginJar.getInputStream( pluginDescriptorEntry ) )
Review comment:
In line 232 is the bug @Tibor17 has described.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] Tibor17 commented on a change in pull request #669: Use try-with-resources
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #669:
URL: https://github.com/apache/maven/pull/669#discussion_r800091854
##########
File path: maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java
##########
@@ -216,9 +216,10 @@ private PluginDescriptor extractPluginDescriptor( Artifact pluginArtifact, Plugi
if ( pluginDescriptorEntry != null )
{
- InputStream is = pluginJar.getInputStream( pluginDescriptorEntry );
-
- pluginDescriptor = parsePluginDescriptor( is, plugin, pluginFile.getAbsolutePath() );
+ try ( InputStream is = pluginJar.getInputStream( pluginDescriptorEntry ) )
Review comment:
This is wrong.
The stream is closed twice.
The callers must not close the stream. It can be done only within the private method `parsePluginDescriptor()` and not here. The method `parsePluginDescriptor()` wraps the stream and so the `Reader` has to close it.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] garydgregory commented on a change in pull request #669: Use try-with-resources
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #669:
URL: https://github.com/apache/maven/pull/669#discussion_r800233008
##########
File path: maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java
##########
@@ -216,9 +216,10 @@ private PluginDescriptor extractPluginDescriptor( Artifact pluginArtifact, Plugi
if ( pluginDescriptorEntry != null )
{
- InputStream is = pluginJar.getInputStream( pluginDescriptorEntry );
-
- pluginDescriptor = parsePluginDescriptor( is, plugin, pluginFile.getAbsolutePath() );
+ try ( InputStream is = pluginJar.getInputStream( pluginDescriptorEntry ) )
Review comment:
The issue, from my POV, is that I think it is best for the method that allocates a resource to release it, and to so do "finally" with a try-with-resources statement. There are exceptions of course. What I personally do not favor is the style that I see here and there in Maven where one method allocates a stream and another method deep in the code path closes the stream; to me, this is an anti-pattern.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] Tibor17 commented on a change in pull request #669: Use try-with-resources
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #669:
URL: https://github.com/apache/maven/pull/669#discussion_r800091854
##########
File path: maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java
##########
@@ -216,9 +216,10 @@ private PluginDescriptor extractPluginDescriptor( Artifact pluginArtifact, Plugi
if ( pluginDescriptorEntry != null )
{
- InputStream is = pluginJar.getInputStream( pluginDescriptorEntry );
-
- pluginDescriptor = parsePluginDescriptor( is, plugin, pluginFile.getAbsolutePath() );
+ try ( InputStream is = pluginJar.getInputStream( pluginDescriptorEntry ) )
Review comment:
This is wrong.
The caller must not close the stream. It can be done only in the private method `parsePluginDescriptor()` and not here.
The method wraps the stream and so the `Reader` has to close it.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #669: Use try-with-resources
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #669:
URL: https://github.com/apache/maven/pull/669#discussion_r800231885
##########
File path: maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java
##########
@@ -216,9 +216,10 @@ private PluginDescriptor extractPluginDescriptor( Artifact pluginArtifact, Plugi
if ( pluginDescriptorEntry != null )
{
- InputStream is = pluginJar.getInputStream( pluginDescriptorEntry );
-
- pluginDescriptor = parsePluginDescriptor( is, plugin, pluginFile.getAbsolutePath() );
+ try ( InputStream is = pluginJar.getInputStream( pluginDescriptorEntry ) )
Review comment:
@gnodet You are right. Let's make it consistently correct.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] Tibor17 commented on a change in pull request #669: Use try-with-resources
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #669:
URL: https://github.com/apache/maven/pull/669#discussion_r800091854
##########
File path: maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java
##########
@@ -216,9 +216,10 @@ private PluginDescriptor extractPluginDescriptor( Artifact pluginArtifact, Plugi
if ( pluginDescriptorEntry != null )
{
- InputStream is = pluginJar.getInputStream( pluginDescriptorEntry );
-
- pluginDescriptor = parsePluginDescriptor( is, plugin, pluginFile.getAbsolutePath() );
+ try ( InputStream is = pluginJar.getInputStream( pluginDescriptorEntry ) )
Review comment:
This is wrong.
The caller must not close the stream. It can be done on in the private method `parsePluginDescriptor()` and not here.
The method wraps the stream and so the `Reader` has to close it.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] garydgregory commented on pull request #669: Use try-with-resources
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #669:
URL: https://github.com/apache/maven/pull/669#issuecomment-1030681893
ping :-)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] Tibor17 commented on a change in pull request #669: Use try-with-resources
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #669:
URL: https://github.com/apache/maven/pull/669#discussion_r800232604
##########
File path: maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java
##########
@@ -216,9 +216,10 @@ private PluginDescriptor extractPluginDescriptor( Artifact pluginArtifact, Plugi
if ( pluginDescriptorEntry != null )
{
- InputStream is = pluginJar.getInputStream( pluginDescriptorEntry );
-
- pluginDescriptor = parsePluginDescriptor( is, plugin, pluginFile.getAbsolutePath() );
+ try ( InputStream is = pluginJar.getInputStream( pluginDescriptorEntry ) )
Review comment:
@gnodet
It is not about the low level of `close` method.
The problem is that the second close is redundant.
And the most important thing is that the straem is wrapped by `Reader` in the method `parsePluginDescriptor()` and the method should close the `Reader`. The caller is only dispatching the stream and that's the whole thing and the caller of the method `parsePluginDescriptor()` does not need to do anything about closing the nested stream. Why to do it twice, makes no sense!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] Tibor17 commented on a change in pull request #669: Use try-with-resources
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on a change in pull request #669:
URL: https://github.com/apache/maven/pull/669#discussion_r800233649
##########
File path: maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java
##########
@@ -216,9 +216,10 @@ private PluginDescriptor extractPluginDescriptor( Artifact pluginArtifact, Plugi
if ( pluginDescriptorEntry != null )
{
- InputStream is = pluginJar.getInputStream( pluginDescriptorEntry );
-
- pluginDescriptor = parsePluginDescriptor( is, plugin, pluginFile.getAbsolutePath() );
+ try ( InputStream is = pluginJar.getInputStream( pluginDescriptorEntry ) )
Review comment:
@garydgregory
It does not matter that one method is producer and another is consumer. The reason is that both are private methods and still such private methods have to by analysed. The private methods only simplify the reading abilities of the code because human brain is not able to analyse whole inlined code. Of course you can parse `pluginJar` as a method parameter into the method `parsePluginDescriptor()` but in that case we are not talking only about `try with resource` but we are talking about refactoring shich is a subject of another PR.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] gnodet commented on a change in pull request #669: Use try-with-resources
Posted by GitBox <gi...@apache.org>.
gnodet commented on a change in pull request #669:
URL: https://github.com/apache/maven/pull/669#discussion_r800231267
##########
File path: maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java
##########
@@ -216,9 +216,10 @@ private PluginDescriptor extractPluginDescriptor( Artifact pluginArtifact, Plugi
if ( pluginDescriptorEntry != null )
{
- InputStream is = pluginJar.getInputStream( pluginDescriptorEntry );
-
- pluginDescriptor = parsePluginDescriptor( is, plugin, pluginFile.getAbsolutePath() );
+ try ( InputStream is = pluginJar.getInputStream( pluginDescriptorEntry ) )
Review comment:
I tend to disagree because the `Closeable#close()` method is explicitely described as being idempotent. Else, the same applies to the call to `builder.build()` inside the `parsePluginDescriptor` which also closes the given stream.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org