[GitHub] [beam] lostluck commented on a change in pull request #15815: [BEAM-13130] Remove persistent references to stateKeyReaders

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

lostluck commented on a change in pull request #15815:
URL: https://github.com/apache/beam/pull/15815#discussion_r737589733



##########
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:
       So this PR isn't ensuring all readers are closed in user panic/error cases like the flawed earlier approach does. A user panic or a dofn returning an error will leave readers open, causing other leaks.
   
   Something also needs to change in exec/pardo.go to close any open streams in that case, using a defer to cover panic handling cases.
   
   Essentially current change only works for happy path cases.




-- 
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: github-unsubscribe@beam.apache.org

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