[GitHub] [cxf] Sevyls opened a new pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] Sevyls opened a new pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls

GitBox

Sevyls opened a new pull request #771:
URL: https://github.com/apache/cxf/pull/771


   see https://issues.apache.org/jira/browse/CXF-8442


--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls

GitBox

reta commented on a change in pull request #771:
URL: https://github.com/apache/cxf/pull/771#discussion_r609123294



##########
File path: core/src/main/java/org/apache/cxf/io/CachedOutputStream.java
##########
@@ -225,7 +225,11 @@ public void close() throws IOException {
             ciphers.clean();
         }
         if (!maybeDeleteTempFile(currentStream)) {
-            postClose();
+            try {

Review comment:
       @Sevyls thank you for the investigation and the consequent suggested fix. I think we should attack the problem from the different angle (as you also mentioned in the ticket):
    - `postClose()` could raise `IOException` so does `close()` and this is legitimate, no need to catch it
    - however, as you found out, `CacheAndWriteOutputStream` may attempt to close the underlying stream many times
   I think the proper solution would be:
    - `CacheAndWriteOutputStream`  should keep track if the `flowThroughStream` has been closed already
    - `closeFlowthroughStream()` and `postClose()` should set or/and check if the `flowThroughStream` has been closed already and behave appropriately (flush/close or do nothing)
   
   What do you think?




--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls

GitBox
In reply to this post by GitBox

reta commented on a change in pull request #771:
URL: https://github.com/apache/cxf/pull/771#discussion_r609123294



##########
File path: core/src/main/java/org/apache/cxf/io/CachedOutputStream.java
##########
@@ -225,7 +225,11 @@ public void close() throws IOException {
             ciphers.clean();
         }
         if (!maybeDeleteTempFile(currentStream)) {
-            postClose();
+            try {

Review comment:
       @Sevyls thank you for the investigation and the consequent suggested fix. I think we should attack the problem from the different angle (as you also mentioned in the ticket):
    - `postClose()` could raise `IOException` so does `close()` and this is legitimate, no need to catch it
    - however, as you found out, `CacheAndWriteOutputStream` may attempt to close the underlying stream many times
   
   I think the proper solution would be:
    - `CacheAndWriteOutputStream`  should keep track if the `flowThroughStream` has been closed already
    - `closeFlowthroughStream()` and `postClose()` should set or/and check if the `flowThroughStream` has been closed already and behave appropriately (flush/close or do nothing)
   
   What do you think?




--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] Sevyls commented on pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls

GitBox
In reply to this post by GitBox

Sevyls commented on pull request #771:
URL: https://github.com/apache/cxf/pull/771#issuecomment-815715068


   Hi @reta,
   
   Thanks for your answer and suggestions. I reverted my changes in `CacheAndWriteOutputStream`.
   
   I added a variable isClosed to `CacheAndWriteOutputStream` and as `postClose()` and `closeFlowthroughStream()` had the same implementation I reduced it to a call to `postClose()`.
   
   Please review my pull request again, see the test case `CacheAndWriteOutputStreamTest#testCloseMultipleTimes()`.
   
   
   


--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] Sevyls edited a comment on pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls

GitBox
In reply to this post by GitBox

Sevyls edited a comment on pull request #771:
URL: https://github.com/apache/cxf/pull/771#issuecomment-815715068


   Hi @reta,
   
   Thank for your answer and suggestions. I reverted my changes in `CacheAndWriteOutputStream`.
   
   I added a variable isClosed to `CacheAndWriteOutputStream` and as `postClose()` and `closeFlowthroughStream()` had the same implementation I reduced it to a call to `postClose()`.
   
   Please review my pull request again, see the test case `CacheAndWriteOutputStreamTest#testCloseMultipleTimes()`.
   
   
   


--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls

GitBox
In reply to this post by GitBox

reta commented on a change in pull request #771:
URL: https://github.com/apache/cxf/pull/771#discussion_r610145530



##########
File path: core/src/main/java/org/apache/cxf/io/CacheAndWriteOutputStream.java
##########
@@ -47,19 +47,32 @@ public void setCacheLimit(long l) {
     }
 
     public void closeFlowthroughStream() throws IOException {
-        flowThroughStream.flush();
-        flowThroughStream.close();
+        postClose();
     }
 
     protected void postClose() throws IOException {
-        flowThroughStream.flush();
-        flowThroughStream.close();
+        if (!isClosed) {
+            flowThroughStream.flush();
+            flowThroughStream.close();
+            isClosed = true;
+        }
     }
 
     public OutputStream getFlowThroughStream() {
         return flowThroughStream;
     }
 
+    @Override
+    protected void doClose() throws IOException {
+        super.doClose();
+        isClosed = true;

Review comment:
       Just for the record, probably not necessary (since `close()` would call `postClose()`) but won't hurt




--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls

GitBox
In reply to this post by GitBox

reta commented on a change in pull request #771:
URL: https://github.com/apache/cxf/pull/771#discussion_r610147111



##########
File path: core/src/main/java/org/apache/cxf/io/CacheAndWriteOutputStream.java
##########
@@ -29,7 +29,7 @@
  *
  */
 public class CacheAndWriteOutputStream extends CachedOutputStream {
-
+    private boolean isClosed = false;

Review comment:
       The stream should not be shared between different threads, so we may not need `volatile` or atomic




--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls

GitBox
In reply to this post by GitBox

reta commented on pull request #771:
URL: https://github.com/apache/cxf/pull/771#issuecomment-816261024


   @Sevyls Thank you.


--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta merged pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls

GitBox
In reply to this post by GitBox

reta merged pull request #771:
URL: https://github.com/apache/cxf/pull/771


   


--
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [cxf] reta commented on a change in pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls

GitBox
In reply to this post by GitBox

reta commented on a change in pull request #771:
URL: https://github.com/apache/cxf/pull/771#discussion_r610253259



##########
File path: core/src/main/java/org/apache/cxf/io/CacheAndWriteOutputStream.java
##########
@@ -47,19 +47,32 @@ public void setCacheLimit(long l) {
     }
 
     public void closeFlowthroughStream() throws IOException {
-        flowThroughStream.flush();
-        flowThroughStream.close();
+        postClose();
     }
 
     protected void postClose() throws IOException {
-        flowThroughStream.flush();
-        flowThroughStream.close();
+        if (!isClosed) {
+            flowThroughStream.flush();
+            flowThroughStream.close();
+            isClosed = true;
+        }
     }
 
     public OutputStream getFlowThroughStream() {
         return flowThroughStream;
     }
 
+    @Override
+    protected void doClose() throws IOException {
+        super.doClose();
+        isClosed = true;

Review comment:
       @Sevyls I had to remove `close()` and `doClose()`, the complex relationships between these methods led to the issues that `isClosed` flag being set **before** closing the streams. As the results, streams stayed open.




--
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:
[hidden email]