[GitHub] [daffodil] stevedlawrence commented on a change in pull request #754: WIP: Call setFinished on the correct data output stream

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #754:
URL: https://github.com/apache/daffodil/pull/754#discussion_r806767799



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/unparsers/UState.scala
##########
@@ -344,7 +342,21 @@ abstract class UState(
       // Finished means you won't add data to the end of it any more.
       // It does NOT prevent information like the absoluteBitPos to
       // propagate.
-      dos.setFinished(this)
+      //
+      // Need this clone or we could possibly end up in an infinite loop inside of
+      // DirectOrBuffereDataOutputStream.cleanUp()

Review comment:
       I think there's a better explanation for why we need a clone of the UState, maybe something like this:
   
   > When setFinished is called, the DOS is going to store the state that we pass into it in finishedFormatInfo. At some point in the future, this DOS will become a direct DOS that may be delivered to a following buffered DOS. When that happens, this saved finishedFormatInfo will be used. However, this all requires that the UState does not change while we are waiting for this DOS to become direct. If the state does change, it will be incorrect and can lead to undefined behavior. To prevent this, we must clone the UState so it can no longer change, and pass that clone into setFinished.

##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/unparsers/UState.scala
##########
@@ -344,7 +342,21 @@ abstract class UState(
       // Finished means you won't add data to the end of it any more.
       // It does NOT prevent information like the absoluteBitPos to
       // propagate.
-      dos.setFinished(this)
+      //
+      // Need this clone or we could possibly end up in an infinite loop inside of
+      // DirectOrBuffereDataOutputStream.cleanUp()
+      val finfo = this match {
+        case m: UStateMain => m.cloneForSuspension(dos)
+        case _ => this

Review comment:
       I wonder if this case is even possible? If we are adding a new buffered DOS below, I think that means that this must be a `UStateMain`--we can't split a DOS that is associated with an already cloned UState since that means the UState is from a suspension which must have already been split.  I think `this` *must* be a `UStateMain` and that it *must* be cloned. I think we can just change `this` to something like `Assert.invariantFailed("state must be a UStateMain when splitting for bit order change")`

##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/unparsers/UState.scala
##########
@@ -344,7 +342,21 @@ abstract class UState(
       // Finished means you won't add data to the end of it any more.
       // It does NOT prevent information like the absoluteBitPos to
       // propagate.
-      dos.setFinished(this)
+      //
+      // Need this clone or we could possibly end up in an infinite loop inside of
+      // DirectOrBuffereDataOutputStream.cleanUp()
+      val finfo = this match {
+        case m: UStateMain => m.cloneForSuspension(dos)
+        case _ => this
+      }
+
+      val newDOS = dos.addBuffered
+      // NOTE: We must call set finished on the original dos with the
+      // UState/finfo before we set this.dataOutputStream to the newly split
+      // DOS, otherwise the call to this.dataOutputStream = newDOS will update the
+      // formatInfo to point to the new DOS when we try to call setFinished.
+      dos.setFinished(finfo)
+      dataOutputStream = newDOS

Review comment:
       I think the order that this happens as mentioned in the comment no longer matters. As long as we call `cloneForSuspension` before we do `dataOutputStream = newDOS` and modify this state this should all work as expected. This is because `setFinished` will use our clone so changing the `dataOutputStream` of this `UState` no longer has any affect like it does now (which was part of the issue). I'm not sure if one order is preferred over the other, but I don't think the comment is needed if we add something similar to the comment mentioned above.




-- 
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: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org