Skip to content

[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

Merged
merged 18 commits into from
Jun 12, 2025

Conversation

javiercn
Copy link
Member

@javiercn javiercn commented Jun 6, 2025

Adds support for triggering pauses from the circuit proactively from JavaScript

@javiercn javiercn marked this pull request as ready for review June 6, 2025 19:20
@javiercn javiercn requested a review from a team as a code owner June 6, 2025 19:20
@javiercn javiercn force-pushed the javiercn/persistent-component-state-graceful branch from dcfe665 to 4ae47fd Compare June 8, 2025 22:12
Comment on lines 34 to 43
if (saveStateToClient)
{
await SaveStateToClient(circuit, store.PersistedCircuitState, cancellation);
}
else
{
await circuitPersistenceProvider.PersistCircuitAsync(
circuit.CircuitId,
store.PersistedCircuitState,
cancellation);
}
Copy link
Member

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);
Copy link
Member

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()?

Copy link
Member Author

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)

Copy link
Member

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 🙂

Copy link
Member Author

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

Comment on lines 288 to 294
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;
Copy link
Member

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?

Copy link
Member Author

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

Comment on lines +44 to +45
isClientPause,
remotePause,
Copy link
Member

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?

Copy link
Member Author

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.

Comment on lines +141 to +152
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;
});
Copy link
Member

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?

Copy link
Member Author

@javiercn javiercn Jun 10, 2025

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)

Copy link
Member Author

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.

Copy link
Member

@MackinnonBuck MackinnonBuck Jun 10, 2025

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?

Copy link
Member Author

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).

@javiercn javiercn force-pushed the javiercn/persistent-component-state-ungraceful branch 2 times, most recently from cf5a455 to aea83a7 Compare June 10, 2025 09:57
@javiercn javiercn force-pushed the javiercn/persistent-component-state-graceful branch from 4ae47fd to 816f93b Compare June 10, 2025 10:59
@javiercn javiercn requested a review from a team June 10, 2025 15:13
Copy link
Member

@pavelsavara pavelsavara left a 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
Copy link
Member

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?

Copy link
Member Author

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 () => {
Copy link
Member

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()

Copy link
Member Author

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);
Copy link
Member

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" ?

Copy link
Member Author

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();
Copy link
Member

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.

Copy link
Member Author

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) {
Copy link
Member

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 ?

Copy link
Member Author

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.

@javiercn javiercn force-pushed the javiercn/persistent-component-state-ungraceful branch from aea83a7 to 8582da8 Compare June 11, 2025 13:46
@javiercn javiercn closed this Jun 11, 2025
@javiercn javiercn force-pushed the javiercn/persistent-component-state-graceful branch from f17b908 to 8582da8 Compare June 11, 2025 13:48
@javiercn javiercn reopened this Jun 11, 2025
Base automatically changed from javiercn/persistent-component-state-ungraceful to main June 11, 2025 17:53
@javiercn javiercn force-pushed the javiercn/persistent-component-state-graceful branch from e5a076c to b6632dd Compare June 11, 2025 19:22
@javiercn javiercn enabled auto-merge (squash) June 11, 2025 19:32
@javiercn javiercn merged commit 16438b6 into main Jun 12, 2025
27 checks passed
@javiercn javiercn deleted the javiercn/persistent-component-state-graceful branch June 12, 2025 00:21
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview6 milestone Jun 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants