lostluck commented on a change in pull request #15815:
URL: https://github.com/apache/beam/pull/15815#discussion_r737664916
##########
File path: sdks/go/pkg/beam/core/runtime/harness/statemgr.go
##########
@@ -107,10 +105,6 @@ func (s *ScopedStateReader) Close() error {
s.mu.Lock()
s.closed = true
s.mgr = nil
- for _, r := range s.opened {
- r.Close() // force close all opened readers
Review comment:
Hmmm might be fine then, assuming we defer the postInvoke instead of
what we're currently doing. Essentially even if a user returns an error, it's
still possible for user code to panic, or for the framework to convert a user
error to a panic. So panics are inevitable, which means cleanups must happen in
a defer to guarantee execution.
All the ReusableInput wrappers seem to close the streams on Reset(), except
for ReIterables which don't track it's the iterators it's spawned out. This
applies to both the
[reflective](https://github.com/apache/beam/blob/e373d92f3e89396971038072e0e0d2489764ea30/sdks/go/pkg/beam/core/runtime/exec/input.go#L89)
versions and the non-reflective code generated versions.
So if we do do the deferred cleanup, we move leak risk from All Side Inputs
to just Re-Iterables, on bundle failure. I think that's a fair trade off if we
call it out, and largely moot if the Cache is in effect. I think that's
probably OK. Note that Map Side Inputs would also be similarly at risk, during
bundle failure.
Regarding ScopedStateReader's Close, it's happening in the harness layer,
which is insulated from user errors and panics since that path is called via a
`callNoPanic` wrapper.
https://github.com/apache/beam/blob/0a950972d1fad8c307e1bb463ece1c61b67ec230/sdks/go/pkg/beam/core/runtime/exec/plan.go#L118
This way it's always going to run, and failing bundles weren't going to leak
anything.
--
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]
