Fix issue #2724 and forceReadonly API while connected

[curtisman]

• Improve readability of documentDirty tests
• Add assert that we only get event when state changes (i.e. no double dirty or double save event)
• Fix issue Avoid multiple dirty->clean->dirty transition event sent on processing ops after reconnection #2724 by making the disable emitting the event while replay.
• forceReadonly(true) can't be set when it is connected. So the API will implicit disconnect and reconnect if it was connected.
• Added test with forceReadonly, replay and document dirty state. Identified and fixed replay op on changing force read only mode
  • There was an issue that documentDirty bit is clear but not being replayed when we transitioning to force read only mode.
    • The fix is only to "clear" the flags if we are going to replay.
  • replay is different transition out of force readonly mode, where we don't clear the dirty flag first. So if there is a DDS that might cancel a pending up while replaying ops, we will not clear the dirty flag.
    • The fix is reuse the same function in both case so they have the same behavior.

[msfluid-bot]

⯅ @fluidframework/base-host: +204 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
main.js	163.62 KB	163.82 KB	⯅ +204 Bytes
Total Size	163.62 KB	163.82 KB	⯅ +204 Bytes

⯅ @fluid-example/bundle-size-tests: +235 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
container.js	181.17 KB	181.4 KB	⯅ +235 Bytes
map.js	39.78 KB	39.78 KB	■ No change
matrix.js	137.43 KB	137.43 KB	■ No change
odspDriver.js	193.98 KB	193.98 KB	■ No change
sharedString.js	151.05 KB	151.05 KB	■ No change
Total Size	703.42 KB	703.64 KB	⯅ +235 Bytes

---

Baseline commit: b289288

Generated by 🚫 dangerJS against b3bc120

[vladsud]

I'm not certain reconnection is right flow.
We might be already connected as "read"
Also re-connecting as "read" can still give you "write" connection - server is free to give you more.

Also this._reconnectMode might be ReconnectMode.Never or ReconnectMode.Disabled, in such case we should not reconnect (this is checked in reconnectOnError(), which probably should be duplicated via assert connectCore(), but it also likely point out that we are better off using reconnectOnError() flow vs. manually disconnecting and reconnecting.

I'd rather either scope the usage of this API to disconnected state, OR not touch connection, but operate only with readonly property. Latter is simpler and feels better, but it breaks the assumption that we somewhat ride on here - that connection properties (like permissions, read-only, etc.) are changed only on granularity of connections.

[curtisman]

The point is not to switch the mode here, but we have to disconnect to switch to readonly, I can't use reconnectOnError here, because I needs to switch the readonly mode in between.

With this change, it would only reconnect if we switch from write to read, and it really does matter what permission the server give us. We don't rely on that to stop sending ops to the server, but explicity depend on the readonly flag.

Not touching the connection is where we have problems with the assert in the readonly event handler, because we can't replay the ops after we reconnect on the same connection.

Logically, this is an internal disconnect reconnect, not auto reconnect which I thought the reconnectMode is supposed to control. What is the usage of reconnectMode?

[vladsud]

It makes sense. One thing to consider though - this is not fully invisible to users. For example, if that functionality would have been applied to summarizing container, then it would puke, because it notices disconnects and stops.
But I guess we can leave with it.

[curtisman]

True..... may be we can disable this API for summarizing clients? I will leave it for now.

[vladsud]

not sure if you check anywhere, if not, can you please check that container is actually in read-only state after flipping true and is not after false?
The best way to validate it is to have a component that reports last seen "readonly" event on datastore runtime (I think)

[curtisman]

@vladris has a PR for some forceReadonly specific test. I will leave it to be added there.

[agarwal-navin]

The document dirty event part looks good to me.