Add stricter assertions regarding stream state. #812
Conversation
| FSTLog(@"%@ Ignoring server close for already closed stream.", NSStringFromClass([self class])); | ||
| return; | ||
| } | ||
| FSTAssert([self isStarted], @"handleStreamClose: called for non-started stream."); |
There was a problem hiding this comment.
I don't think this is safe at all.
PR #811 was defending against gRPC calling us and enqueueing an error while (or just before) we were already processing a close for a different reason. This pushed the check in the callback filter out onto the dispatch queue making the callback filter effective again and prevents callbacks from gRPC from getting us after the stream is closed.
However, it's still the case that an idle close timer firing, a user disableNetwork, and a gRPC error can be enqueued at the same time. The first one to succeed may now prevent gRPC from kicking out any further events, but the subsequent close actions are still going to pass through here.
There was a problem hiding this comment.
Agree to disagree.
Whichever of those 3 are executed first will call closeWithFinalState: which will call suppressCallbacks in conjunction with setting state to a non-started value, which will prevent subsequent calls to handleStreamClose:.
The only other caller of handleStreamClose: is resumeStartWithToken: when handling auth failures, but it asserts that self.state == FSTStreamStateAuth immediately prior to calling handleStreamClose: so this assert is safe in that case too.
There was a problem hiding this comment.
Said differently, we always suppress callbacks when setting the stream state to a non started state... And with your change we can now rely on that to prevent any subsequent handleStreamClose calls.
There was a problem hiding this comment.
Okay, I've convinced myself you're right by tracing it through. All callers check the state prior to calling so this is safe.
| FSTLog(@"%@ Ignoring stream message from inactive stream.", NSStringFromClass([self class])); | ||
| return; | ||
| } | ||
| FSTAssert([self isStarted], @"writeValue: called for stopped stream."); |
There was a problem hiding this comment.
In contrast to the above, this and below should be safe again because they're only called by gRPC.
| FSTLog(@"%@ Ignoring server close for already closed stream.", NSStringFromClass([self class])); | ||
| return; | ||
| } | ||
| FSTAssert([self isStarted], @"handleStreamClose: called for non-started stream."); |
There was a problem hiding this comment.
Okay, I've convinced myself you're right by tracing it through. All callers check the state prior to calling so this is safe.
* Use -[GRPCCall setResponseDispatchQueue] to dispatch GRPC callbacks directly onto the Firestore worker queue. This saves a double-dispatch and simplifies our logic. * Add stricter assertions regarding stream state. (#812)
In the interest of going for the PRs-based-on-PRs world record, I additionally would like to replace some of our race condition leniency with stricter asserts. I've traced the call sites for each assert and feel pretty confident that they should be safe now.