[GitHub] [daffodil] mbeckerle commented on a diff in pull request #841: Ensure layers work well with suspensions and bitOrder

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

mbeckerle commented on code in PR #841:
URL: https://github.com/apache/daffodil/pull/841#discussion_r972279714


##########
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/LayeredSequenceUnparser.scala:
##########
@@ -35,29 +35,55 @@ class LayeredSequenceUnparser(ctxt: SequenceRuntimeData,
   override def unparse(state: UState): Unit = {
     val layerTransformer = layerTransformerFactory.newInstance(layerRuntimeInfo)
 
-    val originalDOS = state.dataOutputStream // layer will output to the original, then finish it upon closing.
+    val originalDOS = state.dataOutputStream
+
+    // create a new buffered DOS that this layer will flush to when the layer
+    // completes unparsing. This ensures that any fragment bits or bitOrder
+    // state in the original DOS does not affect how the layer flushes bytes to
+    // the underlying DOS. Only when this new DOS is delivered to the
+    // originalDOS will the bitOrder checks be done.
+    val layerUnderlyingDOS = originalDOS.addBuffered()
+
+    // clone the UState to be used when the layer flushes its buffered content
+    // to layerUnderlyingDOS. This layer isn't a suspension, but this gives
+    // us an immutable clone that the layer can safely use
+    val finfoPre = state.asInstanceOf[UStateMain].cloneForSuspension(layerUnderlyingDOS)
+
+    // mark the original DOS as finished--no more data will be unparsed to it.
+    // If known, this will carry bit position forward to the layerUnerlyingDOS,
+    // or could even make that DOS direct. Note that we must finish with the
+    // cloned state to ensure we use this state snapshot when the
+    // layerUnderlyingDOS is delivered to it
+    originalDOS.setFinished(finfoPre)
+
+    // create a new DOS where unparsers following this layer will unparse
+    val layerFollowingDOS = layerUnderlyingDOS.addBuffered()
 
-    val newDOS = originalDOS.addBuffered() // newDOS is where unparsers after this one returns will unparse into.
-    //
     // New layerDOS is where the layer will unparse into. Ultimately anything written
-    // to layerDOS ends up, post transform, in originalDOS.
-    //
-    val layerDOS = layerTransformer.addLayer(originalDOS, state)
+    // to layerDOS ends up, post transform, in layerUnderlyingDOS
+    val layerDOS = layerTransformer.addLayer(layerUnderlyingDOS, state, finfoPre)
 
     // unparse the layer body into layerDOS
     state.dataOutputStream = layerDOS
     super.unparse(state)
     layerTransformer.endLayerForUnparse(state)
 
-    // now we're done with the layer, so finalize the layer
-    layerDOS.lastInChain.setFinished(state)
+    // now we're done with the layer, so finalize the last DOS in the chain.
+    // Note that we must reclone the UState (it probably changed since our last
+    // clone) and use that when finishing, like before, to ensures that the DOS
+    // stores a state snapshot to be used later if suspensions prevent
+    // completing right now
+    val layerDOSLast = layerDOS.lastInChain
+    val finfoPost = state.asInstanceOf[UStateMain].cloneForSuspension(layerDOSLast)

Review Comment:
   The logic for why this additional state clone is needed is unclear to me. 
   
   Can you explain this any better? When you say "it probably changed since our last clone" - certainly the position has changed. Other things may have changed during unparsing. Eg., bit Order. But I don't get why we need to preserve that. Why do we need this finfoPost object at all? When we setFinished don't we read out of the UState everything we need to do the finish operation? 



-- 
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