[jira] [Comment Edited] (OAK-6707) TarWriter.close() must not throw an exception on subsequent invocations

["Francesco Mari (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/OAK-6707?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16380483#comment-16380483 ] 

Francesco Mari edited comment on OAK-6707 at 2/28/18 3:37 PM:
--------------------------------------------------------------

[~mduerig], this looks better to me:

{noformat}
--- oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/tar/TarWriter.java	(revision 1825558)
+++ oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/tar/TarWriter.java	(date 1519832083000)
@@ -201,7 +201,9 @@
         // this part, as no other synchronized methods should get invoked
         // once close() has been initiated (see related checkState calls).
         synchronized (this) {
-            checkState(!closed);
+            if (closed) {
+                return;
+            }
             closed = true;
         }
{noformat}

This way, we avoid performing the flishing and closing logic further down the method.


was (Author: frm):
[~mduerig], this looks better to me:

{noformat}
--- oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/tar/TarWriter.java	(revision 1825558)
+++ oak-segment-tar/src/main/java/org/apache/jackrabbit/oak/segment/file/tar/TarWriter.java	(date 1519832083000)
@@ -201,7 +201,9 @@
         // this part, as no other synchronized methods should get invoked
         // once close() has been initiated (see related checkState calls).
         synchronized (this) {
-            checkState(!closed);
+            if (closed) {
+                return;
+            }
             closed = true;
         }
{noformat}

This way, we avoid peforming the flushng and closing logic further down the method.

> TarWriter.close() must not throw an exception on subsequent invocations
> -----------------------------------------------------------------------
>
>                 Key: OAK-6707
>                 URL: https://issues.apache.org/jira/browse/OAK-6707
>             Project: Jackrabbit Oak
>          Issue Type: Bug
>          Components: segment-tar
>            Reporter: Michael Dürig
>            Assignee: Michael Dürig
>            Priority: Minor
>              Labels: technical_debt
>             Fix For: 1.9.0, 1.10
>
>
> Invoking TarWriter.close() on an already closed writer throws an {{ISE}}. According to the general contract this is not allowed:
> {code}
> * Closes this stream and releases any system resources associated
> * with it. If the stream is already closed then invoking this
> * method has no effect.
> {code}
> We should adjust the behvaviour of that method accordingly. 
> Failing to comply with that general contract causes {{TarWriter}} instances to fail in try-resource statements when multiple wrapped streams are involved.
> Consider 
> {code}
> try (
>     StringWriter string = new StringWriter();
>     PrintWriter writer = new PrintWriter(string);
>     WriterOutputStream out = new WriterOutputStream(writer, Charsets.UTF_8))
> {
>     dumpHeader(out);
>     writer.println("----------------------------------------");
>     dumpHex(out);
>     writer.println("----------------------------------------");
>     return string.toString();
> }
> {code}
> This code would cause exceptions to be thrown if e.g. the {{PrintWriter.close}} method would not be idempotent. 



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)