Wait for a computable target length before unparsing truncating strings - #1738
olabusayoT wants to merge 1 commit into
Conversation
A string with dfdl:truncateSpecifiedLengthString whose dfdl:length is an expression reading an outputValueCalc element failed with an unretried InfosetNoDataException. The truncating string unparsers evaluate the target length inline, while the outputValueCalc was still blocked on a following sibling, so the error escaped the suspension machinery instead of being retried once the outputValueCalc could resolve. Make the truncating string unparsers SuspendableUnparsers. Their new StringTruncationSuspendableOperation tests that the target length can be evaluated and then runs the string unparse as its continuation. truncatedString_2 now unparses. truncatedString_1 and _4 depend on dfdl:valueLength and remain true deadlocks; their expected diagnostic is now the circular deadlock error. DAFFODIL-1598
stevedlawrence
left a comment
stevedlawrence
left a comment
There was a problem hiding this comment.
+1, interesting pattern we might be able to use more of in the future or if we want to refactor or existing suspensions
| override protected def continuation(ustate: UState): Unit = { | ||
| unparser.unparseString(ustate) | ||
| } | ||
| } |
There was a problem hiding this comment.
This pattern is very interesting. It's not how we usually implement suspensions but I wonder if we should move towards this? So the SuspendableOperation just contains a reference to the unparser and the unparser does the actual work, making it much more similar to how normal unparsers look.
We could make it even a bit more generic. For example, maybe something like this:
class MySuspendableUnparser extends DelegatedSuspendableUnparser {
override protected def suspendableOperation =
new DelegatingSuspendableOperation(this)
override def suspensionTest(ustate: UState) = ...
override def suspensionContinuation(ustate: UState) = ...
...
class DeletgatingSuspendableOperation(
unparser: DelegatedSuspendableUnparser,
) extends SuspendableOperation {
override val rd = unparser.erd
override protected def test(ustate: UState): Boolean =
unparser.suspensionTest(ustate)
override protected def continuation(ustate: UState): Unit =
unparser.suspensionContinuation(ustate)
}So the original unparser implements the test/continuation functions and the DelegatingSuspendableOperation just calls into that. This avoids needing multiple classes for each suspendable unparser as we have now. There's just the original Unparser, with some special functions used by the reuseable DelegatingSuspendableOperation. Note, I'm not a fan of the names, but it gets the point across.
This doesn't work as well if there is additional state that needs to be passed into the SuspendableOperation. I'm not sure if other Unparsers do that. Might not be worth if it they all work differently and need very different state.
Also note something we necessarily need to do now, but might be worth considering in the future.
3 participants
A string with dfdl:truncateSpecifiedLengthString whose dfdl:length is an expression reading an outputValueCalc element failed with an unretried InfosetNoDataException. The truncating string unparsers evaluate the target length inline, while the outputValueCalc was still blocked on a following sibling, so the error escaped the suspension machinery instead of being retried once the outputValueCalc could resolve.
Make the truncating string unparsers SuspendableUnparsers. Their new StringTruncationSuspendableOperation tests that the target length can be evaluated and then runs the string unparse as its continuation.
truncatedString_2 now unparses. truncatedString_1 and _4 depend on dfdl:valueLength and remain true deadlocks; their expected diagnostic is now the circular deadlock error.
DAFFODIL-1598