Closing the write and watch stream after 60s of idleness #388

schmidt-sebastian · 2017-10-18T17:13:14Z

This is port of the Android CLs cr/167321880 and cr/168294709.

This change is unfortunately pretty large:

It ports the Idle timeout logic in RemoteStore and AbstractStream to the iOS equivalent classes.
It moves the initialization of the stream delegate from the stream constructor to 'start'. This allows us to set a new delegate every time we call start.
It allows us to abandon both the delegate as well as the stream. This is useful for streams that we consider closed but that still might send messages.
It adds a new common delegate type that is shared between both the write and the watch stream and forwards calls to the remote store. This allows us to use the delegate in the base class (FSTStream).
To make testing easier and to not have to expose even more of the private API, it gets rid of the beta serializers.
It moves the StreamTest to integration tests and uses the actual streams to tests the handshake semantics.
It adds a new test-only dispatch queue that schedules the idle timeout after one second.

wilhuff

This is a pretty big change and there are a few parts that I'm not a huge fan of.

The first problem is that it conflates the notion of the stream delegate (callbacks for events emitted from the stream) with the GRXWriteable we pass to gRPC. The stream delegate is a part of the API of stream while the writeable is an implementation detail. Let's split these concepts.

I'm also not super wild about bleeding the stream adapting classes into the FSTRemoteStore. This is a wart in the Android implementation that I was hoping to get rid of there. Porting that to iOS seems like a reversion, especially because these classes cannot be anonymous here as they are on Android. We really don't need two layers of adapters between the stream and the remote store: We could define these as abstract dispatchStreamOpen methods on FSTStream and have the FSTWatchStream adapt as required.

If you're up for it you've mixed some non-controversial/structural changes in with the functional one that could be worth splitting into a separate PR (or PRs). For example, consolidating the FSTBetaStreams and rewriting the stream test (before adding additional tests for idleness) could be independent of this. I also understand if you want to press on.

schmidt-sebastian · 2017-10-20T01:13:41Z

I've split up this change into the test rewrite (#391) and this, which is now set to merge against #391 branch (for easier review).

I also split up the delegate handling for GRPC callbacks and Remote Store callbacks and reverted the porting the Stream callback structure from Android. By renaming the handleStreamOpen/handleStreamClose to notify*, I think was able to overcome the part of the design that I did not really fancy.

FYI: I still kept the generic type on FSTStream, since I need to pass this generic delegate type to start:. It doesn't seem like is possible to specify the generic type in the subclasses like it is in Java, so I think as is I don't have any type safety (which might be an argument in favor of just accepting 'id' in start:, but moving the delegate instances back to FSTWatchStream and FSTWriteStream).

googlebot · 2017-10-25T00:06:56Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-10-25T00:24:04Z

CLAs look good, thanks!

schmidt-sebastian · 2017-10-25T00:24:46Z

@wilhuff I merged in the recent changes and updated this PR.

wilhuff · 2017-10-26T17:00:50Z

Firestore/Example/Tests/SpecTests/FSTMockDatastore.m

@@ -76,10 +72,15 @@ - (instancetype)initWithDatabase:(FSTDatabaseInfo *)database

 #pragma mark - Overridden FSTWatchStream methods.

- (void)start {
+- (void)start:(id<FSTWatchStreamDelegate>)delegate {


Labels should always reflect the arguments passed, so this should be startWithDelegate:.

wilhuff · 2017-10-26T17:01:21Z

Firestore/Example/Tests/SpecTests/FSTMockDatastore.m

  [self.delegate watchStreamDidOpen];
 }

+- (void)notifyStreamInterrupted:(NSError *_Nullable)error {


Naming: notifyStreamInterruptedWithError: Also prefer the nullable attribute to the _Nullable qualifier.

This should be

- (void)notifyStreamInterruptedWithError:(nullable NSError *)error

wilhuff · 2017-10-26T17:07:53Z

Firestore/Source/Remote/FSTDatastore.h

-@interface FSTStream : NSObject
+@interface FSTStream <__covariant FSTStreamDelegate> : NSObject
+
+@property (nonatomic, weak, readwrite, nullable) FSTStreamDelegate delegate;


Does the delegate need to be publicly declared? Should anyone outside the class access it? If not, consider just moving this into the implementation.

If someone needs to read it you can declare it readonly here and redeclare it internally as readwrite. Note that conventionally we haven't been marking properties readwrite since that's the default.

It doesn't need to be but it means that we have to redeclare it in the Mock implementations (unless we expose an test only interface)

wilhuff · 2017-10-26T17:08:06Z

Firestore/Source/Remote/FSTDatastore.h

@@ -175,7 +177,7 @@ NS_ASSUME_NONNULL_BEGIN
 *
 * When start returns, -isStarted will return YES.
 */
- (void)start;
+- (void)start:(FSTStreamDelegate)delegate;


Naming: startWithDelegate:

wilhuff · 2017-10-26T17:08:37Z

Firestore/Source/Remote/FSTDatastore.h

 *
 * NOTE: This will not be called after `stop` is called on the stream. See "Starting and Stopping"
 * on FSTStream for details.
 */
- (void)watchStreamDidClose:(NSError *_Nullable)error;
+- (void)watchStreamWasInterrupted:(NSError *_Nullable)error;


Naming: watchStreamWasInterruptedWithError: (the old name was wrong too)

wilhuff · 2017-10-26T17:44:11Z

Firestore/Source/Remote/FSTDatastore.m

@@ -522,14 +572,16 @@ - (void)resumeStartWithToken:(FSTGetTokenResult *)token error:(NSError *)error {
  [FSTDatastore prepareHeadersForRPC:_rpc
                          databaseID:self.databaseInfo.databaseID
                               token:token.token];
-  [_rpc startWithWriteable:self];
+  FSTAssert(_grxFilter == nil, @"GRX Filter must be nil");
+  _grxFilter = [[GRXFilter alloc] initWithStream:self];


Again, the "grx" prefix is something we should avoid. Maybe _callbackFilter?

Renamed to _callbackFilter to match FSTCallbackFilter.

wilhuff · 2017-10-26T17:44:35Z

Firestore/Source/Remote/FSTDatastore.m

 }

 /** Backs off after an error. */
- (void)performBackoff {
+- (void)performBackoff:(id)delegate {


Naming: performBackoffWithDelegate:

wilhuff · 2017-10-26T17:45:01Z

Firestore/Source/Remote/FSTDatastore.m

  }];
 }

 /** Resumes stream start after backing off. */
- (void)resumeStartFromBackoff {
+- (void)resumeStartFromBackoff:(id)delegate {


Naming: resumeStartFromBackoffWithDelegate:

wilhuff · 2017-10-26T17:47:02Z

Firestore/Source/Remote/FSTDatastore.m

+
+  // If the caller explicitly requested a stream stop, don't notify them of a closing stream (it
+  // could trigger undesirable recovery logic, etc.).
+  if (finalState != FSTStreamStateStopped) {


We should avoid sending messages to delegates in the middle of operations like these because they may attempt to perform operations on the stream as a result of the callback. If that happened here the stream would be in an inconsistent state.

I moved some of the clean up logic above. I am not able to move the clearing of the delegate out, since it needs to be available in notifyStreamInterruptedWithError.

wilhuff · 2017-10-26T17:56:33Z

Firestore/Source/Remote/FSTDatastore.m

+    [self.workerDispatchQueue dispatchAsyncAllowingSameQueue:^() {
+      [self handleIdleCloseTimer];
+    }
+                                                       after:kIdleTimeout];


I'm not wild about this API. It's conflating two separate concerns: which queue to dispatch on and when.

This should be more like the backoff implementation: the dispatch queue should be wrapped by the idle timer, and we can supply an alternate implementation of the idle timer for tests. The idle timer could take the dispatch queue and idle timeout as initializer parameters

Okay, we discussed offline, and this does make sense because we're modeling dispatch_after on iOS and a ScheduledExecutorService on Android.

I'm still not crazy about how this API ends up reading (especially because of the way clang format munges it). Maybe if we reversed the argument order it wouldn't look so nuts.

How about dispatchAfter:block:?

Renamed to dispatchAfterDelay:block: and got rid of "allowsamequeue" as well (it's the default).

wilhuff

LGTM

wilhuff · 2017-10-30T21:15:43Z

Firestore/Source/Remote/FSTDatastore.m

+/**
+ * Closes the stream and cleans up as necessary:
+ *
+ * <ul>


Nit: this isn't javadoc, so probably make this a markdown-list.

wilhuff · 2017-10-30T21:17:52Z

Firestore/Source/Remote/FSTDatastore.m

+ * @param error the NSError the connection was closed with.
+ */
+- (void)closeWithFinalState:(FSTStreamState)finalState error:(nullable NSError *)error {
+  FSTAssert(finalState == FSTStreamStateError || error == nil,


This allows the error to be nil if the finalState is FSTStreamStateError. Is that desirable?

This is directly called from GRPC and we don't want to crash the client here if the server cleanly closes our connection for some unknown reason.

googlebot added the cla: yes label Oct 18, 2017

Closing the write and watch stream after 60s of idleness

7280673

schmidt-sebastian force-pushed the mrschmidt-idle branch from eef7419 to 7280673 Compare October 18, 2017 17:16

schmidt-sebastian requested a review from wilhuff October 18, 2017 17:16

schmidt-sebastian assigned wilhuff Oct 18, 2017

wilhuff suggested changes Oct 19, 2017

View reviewed changes

schmidt-sebastian mentioned this pull request Oct 19, 2017

Moving the StreamTest to integration tests #391

Merged

schmidt-sebastian added 3 commits October 19, 2017 10:17

Verifying the write handshake in FSTStreamTest

958f866

Addressing review comments

7e7b82e

Merge

c7b2ccd

schmidt-sebastian changed the base branch from master to mrschmidt-streamtests October 20, 2017 01:00

schmidt-sebastian added 2 commits October 19, 2017 18:00

Clang format

d8b2dfb

Adding class comment

9d745d2

schmidt-sebastian added 2 commits October 19, 2017 18:26

Adding missing files

1c078fe

Cleanup

efae7fc

googlebot added cla: no and removed cla: yes labels Oct 25, 2017

schmidt-sebastian changed the base branch from mrschmidt-streamtests to master October 25, 2017 00:07

Merging recent changes

132e636

schmidt-sebastian force-pushed the mrschmidt-idle branch from f70f00d to 132e636 Compare October 25, 2017 00:24

googlebot added cla: yes and removed cla: no labels Oct 25, 2017

schmidt-sebastian added 2 commits October 25, 2017 19:10

Using the new delay functionality for our GRPC backoff

4c0e49d

Updating method comment

02f182a

wilhuff suggested changes Oct 26, 2017

View reviewed changes

wilhuff removed their assignment Oct 26, 2017

wilhuff assigned schmidt-sebastian Oct 26, 2017

Addressing comments

bbf8b51

schmidt-sebastian removed their assignment Oct 26, 2017

Adding comments.

3cad354

schmidt-sebastian mentioned this pull request Oct 27, 2017

Closing the write and the watch stream after 60s of idleness firebase/firebase-js-sdk#275

Merged

wilhuff approved these changes Oct 30, 2017

View reviewed changes

wilhuff reviewed Oct 30, 2017

View reviewed changes

Using Markdown

0f1916a

schmidt-sebastian merged commit 02ff6bb into master Oct 31, 2017

schmidt-sebastian deleted the mrschmidt-idle branch October 31, 2017 01:21

schmidt-sebastian mentioned this pull request Oct 31, 2017

Moving the Stream class to their own file #418

Merged

minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018

Closing the write and watch stream after 60s of idleness (firebase#388)

e33dbe6

firebase locked and limited conversation to collaborators Nov 12, 2019

Closing the write and watch stream after 60s of idleness #388

Closing the write and watch stream after 60s of idleness #388

Uh oh!

Conversation

schmidt-sebastian commented Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Oct 25, 2017

Uh oh!

googlebot commented Oct 25, 2017

Uh oh!

schmidt-sebastian commented Oct 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schmidt-sebastian commented Oct 18, 2017 •

edited

Loading

schmidt-sebastian commented Oct 20, 2017 •

edited

Loading

schmidt-sebastian Oct 26, 2017 •

edited

Loading

schmidt-sebastian Oct 26, 2017 •

edited

Loading