-
Notifications
You must be signed in to change notification settings - Fork 10.4k
[Blazor] Adds support for persisting and restoring disconnected circuits from storage #62259
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
base: main
Are you sure you want to change the base?
Conversation
7116e55
to
70da07d
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.
Looks great so far - still need to do a deeper review of some parts.
var renderer = circuit.Renderer; | ||
var persistenceManager = circuit.Services.GetRequiredService<ComponentStatePersistenceManager>(); | ||
using var subscription = persistenceManager.State.RegisterOnPersisting( | ||
() => PersistRootComponents(renderer, persistenceManager.State), | ||
RenderMode.InteractiveServer); | ||
var store = new CircuitPersistenceManagerStore(); | ||
await persistenceManager.PersistStateAsync(store, renderer); | ||
|
||
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.
If I'm understanding this correctly:
- We register a new callback to persist root component state
- We then immediately call
PersistStateAsync()
, which invokes the callback - The callback serializes root components into markers (similar to how prerendering works, which makes sense), which then gets serialized into a JSON
byte[]
and stored in theDictionary<string, byte[]>
managed by thePersistentComponentState
- A custom
IPersistentComponentStateStore
then receives the aforementioned dictionary in itsPersistStateAsync()
method, at which point it extracts the root component state from the dictionary and constructs a newDictionary<string, byte[]>
without the root component state
Is this doing more work than necessary? Would it be possible to serialize root component state directly into a JSON byte[]
without inserting it into the PersistentComponentState
's dictionary only to extract it out shortly thereafter? Then we could just directly assign the root component byte[]
to PersistedCircuitState.RootComponents
, and directly assign the PersistentComponentState
dictionary to PersistedCircuitState.ApplicationState
without having to reconstruct it first.
I see that a comment later in this file mentions that the purpose of the callback is to have it run at the same time as other callbacks. Do we expect that removing the callback and instead performing the root component serialization directly after calling PersistStateAsync()
would cause issues? If so, would it work to keep the callback but just not call state.PersistAsJson()
and instead store the root component state separately to avoid reconstructing the dictionary? Totally possible that I'm missing a detail that makes this impractical!
var localRetention = circuitOptions.Value.PersistedCircuitInMemoryRetentionPeriod; | ||
var maxRetention = distributedRetention > localRetention ? distributedRetention : localRetention; | ||
|
||
var marker = ComponentMarker.Create(ComponentMarker.ServerMarkerType, false, componentKey); |
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.
Nit:
var marker = ComponentMarker.Create(ComponentMarker.ServerMarkerType, false, componentKey); | |
var marker = ComponentMarker.Create(ComponentMarker.ServerMarkerType, prerender: false, componentKey); |
private readonly Lock _lock = new(); | ||
private readonly CircuitOptions _options; | ||
private readonly MemoryCache _persistedCircuits; | ||
private readonly Task<PersistedCircuitState> _noMatch = Task.FromResult<PersistedCircuitState>(null); |
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.
Nit: _noMatch
could be static
.
|
||
public CancellationTokenSource TokenSource { get; set; } | ||
|
||
public CircuitId CircuitId { get; set; } |
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 don't see this CircuitId
property being set anywhere. Maybe that's missing from PersistCore()
?
bool TryDeserializeRootComponentOperations(string serializedComponentOperations, [NotNullWhen(true)] out RootComponentOperationBatch? operationBatch); | ||
bool TryDeserializeRootComponentOperations(string serializedComponentOperations, [NotNullWhen(true)] out RootComponentOperationBatch? operationBatch, bool deserializeDescriptors = true); | ||
|
||
public bool TryDeserializeWebRootComponentDescriptor(ComponentMarker record, [NotNullWhen(true)] out WebRootComponentDescriptor? result); |
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 public
modifier isn't necessary
} | ||
|
||
PersistedCircuitState? persistedCircuitState; | ||
if (rootComponents == "[]" && string.IsNullOrEmpty(applicationState)) |
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.
Should this line also be using RootComponentIsEmpty()
like the else if
block below it?
namespace Microsoft.AspNetCore.Components.Server.Circuits; | ||
|
||
// Default implmentation of ICircuitPersistenceProvider that uses an in-memory cache | ||
internal sealed partial class DefaultInMemoryCircuitPersistenceProvider : ICircuitPersistenceProvider |
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.
Should this file name be changed to match the name of this class?
Details to be added. There are some missing things on this PR that will be done in separate PRs:
HybridCache
as a base implementation.The detailed design can be found #60494
The main changes from the original design as well as some additional details are: