Wait to raise Connected event for Read connection until container is caught up

[markfields]

Part of #7884. See comments in connectionStateHandler.ts for description of the desired behavior here.

Note that the new behavior is behind a feature gate "Fluid.Container.CatchUpBeforeDeclaringConnected". I didn't make any changes to existing function waitContainerToCatchUp, even though some functionality is duplicated. Once we finish rolling this out, we can remove the duplicate logic from that function since it will simply wait for the "connected" event (we may even deprecate it at that point)

To-Do:

• Need to update the README and specifically info about CatchingUp state

Some follow-up work items to open afterwards:

• Roll out new feature gate to enable new behavior for Loop partners
• Remove feature gate and deprecate waitContainerToCatchUp

[vladsud]

I think it looks good. Some things missing:

1. Tests
2. breaking.md note
3. Possibly: some feature gate to disable that change, in case we learn it screws up something we did not consider.

[markfields]

Revisiting this finally. Writing down some questions I'm trying to clarify in my head:

• How does startJoinOpTimer work? When it fires, it doesn't do anything. I guess receivedAddMemberEvent keys off whether it's set or not, and if so may call applyForConnectedState which in turn may call this.setConnectionState(ConnectionState.Connected);.
• For write connections, does waitContainerToCatchUp actually cover the scenario of leveraging join op to indicate "caught up"? The comment in there (the one I said I don't understand) seems to indicate so, but I don't see it.

[markfields]

@jatgarg can you take a look too please? Thanks!

[markfields]

Answering my own question...

For write connections, does waitContainerToCatchUp actually cover the scenario of leveraging join op to indicate "caught up"? The comment in there (the one I said I don't understand) seems to indicate so, but I don't see it.

waitContainerToCatchUp gates on connected event which for write connections means we've processed own join op.

This PR kind of inverts this - now always "connected" event means we're caught up. So waitContainerToCatchUp is actually "wait for the container to be connected".

[ghost]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ markfields sign now
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[markfields]

@vladsud, @agarwal-navin, @jatgarg -- Here's my update on this PR, and some input I need from you:

• Do we all agree with the specification that "connected" means "caught up to known ops" even for write clients? i.e. that's an additional delay beyond the "join" op now. Also, the way I've implemented so far the definition of "known ops" varies based on when a particular callback is invoked, so it's a little wishy-washy. Thoughts?
• I started looking into a feature gate for this. Ideally the gate would be used in two places: (1) ConnectionStateHandler to determine when to transition to "connected" state and (2) waitContainerToCatchUp can just wait for "connected" event when the gate is enabled (don't need to wait for additional ops). I don't see a way to cover (2) because it's a function taking in an IContainer, rather than being a private function on Container class. I think this will be fine for waitContainerToCatchUp to essentially catch up twice - if hasCheckpointSequenceNumber it'll be a no-op the second time anyway.
  • If we agree then I'll revert behavior of waitContainerToCatchUp back to how it was.
• I need to write tests
• I need to move this to next and include BREAKING.md note.

Anything else I should consider here? I anticipate moving this from Draft on Monday or Tuesday. Thanks guys!

[markfields]

Remaining tasks:

• Write UTs for CatchUpMonitor and ImmediateCatchUpMonitor
• Fix the skipped test in Container.spec.ts
• Address //* comments

[msfluid-bot]

⯅ @fluid-example/bundle-size-tests: +1.61 KB

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	387.39 KB	387.39 KB	■ No change
connectionState.js	680 Bytes	680 Bytes	■ No change
containerRuntime.js	191.5 KB	191.5 KB	■ No change
loader.js	149.91 KB	151.52 KB	⯅ +1.61 KB
map.js	42.67 KB	42.67 KB	■ No change
matrix.js	127.15 KB	127.15 KB	■ No change
odspDriver.js	149.26 KB	149.26 KB	■ No change
odspPrefetchSnapshot.js	38.31 KB	38.31 KB	■ No change
sharedString.js	147.79 KB	147.79 KB	■ No change
Total Size	1.24 MB	1.24 MB	⯅ +1.61 KB

---

Baseline commit: 4beded1

Generated by 🚫 dangerJS against 4618761

[markfields]

If so, we may introduce additional wait here

What do you all think - is this correct or not? It may not add additional delay since events fire synchronously.

[markfields]

@agarwal-navin @vladsud @tyler-cai-microsoft @jatgarg -- Thanks for the early review! Ready for review now. Just need to add UTs for the new "CatchUpMonitor" classes but otherwise ready to go.

[vladsud]

It feels like instead of using events, it's better to use a property that exposes a promise. I.e. something like

public caughtUp: Promise<number>;

Promise seems to be better because this event files once, and past "firing" reflected on new listeners - that's exactly what promise does.

[markfields]

That is a great point. I don't remember why I switched to event-based to begin with, but I bet it would work better with Promise. That said, I don't have time to try it out now, and also leaving it will reduce merge conflicts with your follow-on refactor.

[vladsud]

Singing off to unblock you - I did not look through it now, but was poking in my prototyping on top of it and overall, it looks fine.

[github-actions]

This commit is queued for merging with the next branch! Please ignore this PR for now. Contact @microsoft/fluid-cr-infra for help.

[markfields]

Opened https://dev.azure.com/fluidframework/internal/_workitems/edit/1384 to track rolling this out