aglinxinyuan opened a new issue, #5660:
URL: https://github.com/apache/texera/issues/5660
## Background
While reviewing #5657, Yicong-Huang flagged the brittle ordering in
`AutoClosingIterator.hasNext`:
`common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/source/scan/AutoClosingIterator.scala`,
lines 35-42:
```scala
override def hasNext: Boolean = {
val hn = iter.hasNext
if (!hn && !alreadyClosed) {
onClose() // <-- if this throws, alreadyClosed stays false
alreadyClosed = true
}
hn
}
```
If `onClose()` throws, `alreadyClosed = true` is never reached. A second
`hasNext` call would see `!alreadyClosed` again and re-invoke `onClose()` —
typically running cleanup twice on a failing resource, which is precisely the
wrong behavior.
`AutoClosingIteratorSpec` (added in #5657) characterizes this current
behavior so a fix is observable in CI. Once the fix lands, that
characterization test ("re-invoke onClose on a retry when the previous onClose
threw") will start failing and should be replaced with a positive assertion of
the new contract.
## What needs to change
Set `alreadyClosed = true` BEFORE invoking `onClose()` so a throwing close
is not retried on the next `hasNext`:
```scala
override def hasNext: Boolean = {
val hn = iter.hasNext
if (!hn && !alreadyClosed) {
alreadyClosed = true
onClose()
}
hn
}
```
The behavior change is:
- Before: a throwing `onClose` is re-invoked on every subsequent `hasNext`.
- After: a throwing `onClose` runs at most once; the exception still
propagates the first time, but a retry surfaces no extra side effect.
## Test plan
After the fix:
1. Update `AutoClosingIteratorSpec` — replace the existing characterization
test ("re-invoke onClose on a retry when the previous onClose threw") with a
positive assertion that a second `hasNext` after a throwing close does NOT
re-invoke `onClose` (closeCount stays at 1).
2. `sbt "WorkflowOperator/testOnly
org.apache.texera.amber.operator.source.scan.AutoClosingIteratorSpec"` — all
should pass.
## Scope
- Single-method edit in `AutoClosingIterator.scala`.
- One updated test case in `AutoClosingIteratorSpec.scala`.
- No callers should observe behavior change unless they were *intentionally*
relying on retry semantics, which would itself be a bug.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
