[Blazor] Adds support for pausing and resuming circuits with support for pushing the state to the client #62271
Conversation
| @@ -14,7 +14,7 @@ | |||
| <HeadOutlet /> | |||
| </head> | |||
| <body> | |||
| <Routes /> | |||
| <Routes @rendermode="InteractiveServer" /> | |||
There was a problem hiding this comment.
Ignore the sample changes. I'll undo them
javiercn
force-pushed
the
javiercn/persistent-component-state-graceful
branch
from
dcfe665 to
4ae47fd
Compare
| public void PauseCircuit() | ||
| { | ||
| _circuitHost.TriggerCircuitPause(); | ||
| } |
There was a problem hiding this comment.
Should this method just be called Pause, since it's containing type is already Circuit, so it's sort of implied that the circuit is the thing being paused?
| if (saveStateToClient) | ||
| { | ||
| await SaveStateToClient(circuit, store.PersistedCircuitState, cancellation); | ||
| } | ||
| else | ||
| { | ||
| await circuitPersistenceProvider.PersistCircuitAsync( | ||
| circuit.CircuitId, | ||
| store.PersistedCircuitState, | ||
| cancellation); | ||
| } |
There was a problem hiding this comment.
From reading this section of code, it wasn't obvious to me that SaveStateToClient actually saves state on the server as a fallback. Do you think it would be clearer if SaveStateToClient was actually TrySaveStateToClient, and the fallback logic is just what's currently in the else block here? Something like:
if (saveStateToClient && await TrySaveStateToClient(...))
{
// State successfully saved to the client - nothing left to do on the server
}
else
{
await circuitPersistenceProvider.PersistCircuitAsync(...);
}| // Root components descriptors are already protected and serialized as JSON, we just convert the bytes to a string. | ||
| var rootComponents = Encoding.UTF8.GetString(state.RootComponents); |
There was a problem hiding this comment.
Could we avoid encoding/decoding root component state by serializing it directly to a string rather than to a byte[] first via PersistedComponentState.PersistAsJson()?
| const newCircuitId = await this._connection!.invoke<string>( | ||
| 'ResumeCircuit', | ||
| this._circuitId, | ||
| navigationManagerFunctions.getBaseURI(), | ||
| navigationManagerFunctions.getLocationHref(), | ||
| '[]', | ||
| '' | ||
| persistedCircuitState?.components ?? '[]', | ||
| persistedCircuitState?.applicationState ?? '', | ||
| ); |
There was a problem hiding this comment.
Sending the state via hub invocation means we can only support up to 32KB of application state by default due to the SignalR message size limit, right? It'll probably be even less than that in reality because of the root component markers. Are we OK with that?
| public async resume(): Promise<boolean> { | ||
| if (!this._circuitId) { | ||
| throw new Error('Method not implemented.'); | ||
| throw new Error('Circuit host not initialized.'); | ||
| } | ||
|
|
||
| if (this._pausingPromise) { | ||
| await this._pausingPromise; |
There was a problem hiding this comment.
Does resume() need to set this._paused back to false?
| isClientPause, | ||
| remotePause, |
There was a problem hiding this comment.
I initially thought the isClientPause and remotePause arguments would always have opposite values because I interpreted them as "client-initiated" vs. "server-initiated", but it's actually "are we gracefully pausing" and "if so, who initiated the pause", right? Maybe names like isGracefulPause and isRemoteInitiator might be clearer?
| connection.on('JS.SavePersistedState', (circuitId: string, components: string, applicationState: string) => { | ||
| if (!this._circuitId) { | ||
| throw new Error('Circuit host not initialized.'); | ||
| } | ||
| if (circuitId !== this._circuitId) { | ||
| throw new Error(`Received persisted state for circuit ID '${circuitId}', but the current circuit ID is '${this._circuitId}'.`); | ||
| } | ||
| this._persistedCircuitState = { components, applicationState }; | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
It looks like _persistedCircuitState is just stored in-memory. I've sometimes seen the Edge browser automatically refresh sleeping tabs, which would cause this state to be lost. Should we consider storing persisted state in e.g., local storage so that we handle cases like this?
javiercn
force-pushed
the
javiercn/persistent-component-state-ungraceful
branch
from
70da07d to
7820b1b
Compare
Missing: