You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by GitBox <gi...@apache.org> on 2021/04/07 19:35:08 UTC
[GitHub] [cxf] Sevyls opened a new pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls
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:
users@infra.apache.org
[GitHub] [cxf] reta commented on a change in pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [cxf] Sevyls edited a comment on pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [cxf] reta commented on a change in pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [cxf] Sevyls commented on pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [cxf] reta commented on a change in pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [cxf] reta commented on pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [cxf] reta commented on a change in pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [cxf] reta merged pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [cxf] reta commented on a change in pull request #771: CXF-8442 Java 16 & CachedOutputStream Fix IOException closed after multiple close() & postClose() calls
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org