-
Notifications
You must be signed in to change notification settings - Fork 10.4k
[Blazor] Adds support for pausing and resuming circuits with support for pushing the state to the client #62271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
dcfe665
to
4ae47fd
Compare
if (saveStateToClient) | ||
{ | ||
await SaveStateToClient(circuit, store.PersistedCircuitState, cancellation); | ||
} | ||
else | ||
{ | ||
await circuitPersistenceProvider.PersistCircuitAsync( | ||
circuit.CircuitId, | ||
store.PersistedCircuitState, | ||
cancellation); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we avoid encoding/decoding root component state by serializing it directly to a string
rather than to a byte[]
first via PersistedComponentState.PersistAsJson()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PersistedComponentState.PersistAsJson()
This will serialize it into a byte array in any case.
I don't think it's super important given that this won't be a super common thing and that we already serialize/stringify 2 or 3 times more than necessary.
I feel that it's better to not do this right now and wait for a time to optimize serialization paths all the way through as a separate piece of work (post 10)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that it's better to not do this right now and wait for a time to optimize serialization paths all the way through as a separate piece of work (post 10)
Sounds good to me 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've filed #62312 to cover going through this stuff across the board
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does resume()
need to set this._paused
back to false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed how all this stuff works because it was incredibly confusing. I've created a class to encapsulate all these state transitions and simplify things.
There are also now more thorough tests that validate that we can disconnect/reconnect gracefully/ungracefully multiple times and with different combinations
isClientPause, | ||
remotePause, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
I actually changed this as I was working on other stuff.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely not local storage, maybe session storage.
If the browser refreshes the entire page, aren't we getting a new DOM anyways? I'm working under the premise that the document remains intact. If a full-page refresh happens then you lose all state (same as it would happen with any other regular web app or SPA for that matter).
If the page gets unloaded from memory, it's not a scenario that we are trying to tackle (at least for now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, for reference.
In Microsoft Edge, a sleeping tab is discarded when it has been inactive for 1.5 days and the AutoDiscardSleepingTabsEnabled policy is enabled. This helps free up memory by completely unloading the tab from the system. When you switch back to a discarded tab, it will need to reload instead of instantly resuming like a regular sleeping tab.
So, I don't think this is an important scenario for the feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the browser refreshes the entire page, aren't we getting a new DOM anyways? I'm working under the premise that the document remains intact.
Yeah, it would be a new DOM, so you wouldn't retain any state that wasn't represented on the server. I was just thinking it could be helpful to persist server-side state even if DOM state was lost, especially when it can be used to re-hydrate the DOM (e.g., two-way bindings). But I think it's fine to consider this out of scope.
In Microsoft Edge, a sleeping tab is discarded when it has been inactive for 1.5 days and the AutoDiscardSleepingTabsEnabled policy is enabled...
So, I don't think this is an important scenario for the feature.
Just to clarify, is it not important because we don't expect customers to resume a session that's 1.5 days old?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it would be a new DOM, so you wouldn't retain any state that wasn't represented on the server. I was just thinking it could be helpful to persist server-side state even if DOM state was lost, especially when it can be used to re-hydrate the DOM (e.g., two-way bindings). But I think it's fine to consider this out of scope.
This feature persists the state for the session. If the browser triggers a full reload of the page is a new session altogether.
If we get feedback about it, I'd be open to reconsider after 10, but I do consider that out of scope for 10. To put it in different terms, this feature works within constraints similar to those of re-connect, it's just "an enhanced version".
It's not meant to do things like preserving state when you press F5 or things like that as how GH and other apps do it. That would be IMO a separate thing that we would build (in that case, rightfully, against sessionStorage).
cf5a455
to
aea83a7
Compare
4ae47fd
to
816f93b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just questions, no need to answer them in today ...
// * If that succeeds, the client receives the state and we are done. | ||
// * If that fails, we will fall back to the server-side cache storage. | ||
// * The client will disconnect after receiving the state or after a 30s timeout. | ||
// * From that point on, it can choose to resume the circuit by calling ResumeCircuit with or without the state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if client calls the resume()
in the middle of pending pause->push to client workflow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We push to the client when the client initiates the pause explicitly, so at that point the client must have called pause()
which resume()
is aware of and will await until pause()
completes before trying the resume()
operation.
@@ -67,6 +67,20 @@ async function startServerCore(components: RootComponentManager<ServerComponentD | |||
return true; | |||
}; | |||
|
|||
Blazor.pause = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C# is PauseCircuit
, should it match ? Same for JS resume()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The methods in the SignalR hub are a private implementation detail, we didn't make them match in the past (for example we have reconnect
and ConnectCircuit
.
That said, I think it's worth changing the names for pause
and resume
to something more concrete. Since this will have to go through API review, I will hold on renaming it for now, but I'll cover it as part of the API review.
|
||
// Notify the reconnection handler that we are pausing the circuit. | ||
// This is used to trigger the UI display. | ||
this._options.reconnectionHandler?.onConnectionDown(this._options.reconnectionOptions, undefined, true, remote); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this behavior "breaking change" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, in the sense that is a new feature.
If you have a custom reconnectionHandler and you call pause()
in your code is up to you to make sure that your handler is capable of handling it.
The default handler (which is by far the most common case) will handle this automatically, same as the built-in UI. If you had a custom UI on previous versions it will behave as the reconnect UI
if (this._resumingState.isInprogress()) { | ||
// If we are already resuming, wait for the current resume operation to complete. | ||
this._logger.log(LogLevel.Trace, 'Waiting for the circuit to finish resuming...'); | ||
return this._resumingState.currentProgress(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could resuming fail at this point ? We return from the method and reconnect code below would not run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only one that sets up the resuming promise is this method. The way this works is that the first time you call it, we set the state, the second time you call it, you get the promise created in the first call. (for example if you call resume(); resume();
you get the same promise back both times)
} | ||
|
||
public transitionTo(newState: T): void { | ||
if (this._promise) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there any invalid transitions on the graph ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope.
This helper class roughly encapsulates the transitions to make thins "saner" when we set them up in the CircuitManager. The CircuitManager
takes care of transitioning the right way.
aea83a7
to
8582da8
Compare
f17b908
to
8582da8
Compare
e5a076c
to
b6632dd
Compare
Adds support for triggering pauses from the circuit proactively from JavaScript