Skip to content
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

Breaking change: Blazor StateManager does not use the provided timeout parameter #3800

Closed
StefanOssendorf opened this issue Apr 16, 2024 · 4 comments · Fixed by #3910
Closed

Comments

@StefanOssendorf
Copy link
Contributor

private async Task GetState(TimeSpan timeout)
{
Session session;
var isBrowser = OperatingSystem.IsBrowser();
if (isBrowser)
session = await _sessionManager.RetrieveSession();
}

As shown the timeout parameter is not used in this method. The method is called by the two InitializeAsync() methods where one provides a timeout parameters as well.

I think we have two ways to correct this:

  1. Remove the timeout parameter and remove the InitializeAsync(TimeSpan) method.
  2. We extend ISessionManager.RetrieveSession() with an overload which also accepts a timeout and pass it along

I think option 2 would be the best so the user can decide whether he wants to wait or not.
In this regard: Should the SaveState also get a timeout parameter with passing along to SendSession()?

@rockfordlhotka
Copy link
Member

This is left over from the original design before the final 8.0.0 release.

I suppose a timeout still is a valid concern. And for SaveState as well.

@luizfbicalho
Copy link
Contributor

This is left over from the original design before the final 8.0.0 release.

I suppose a timeout still is a valid concern. And for SaveState as well.

I'm working on this issue, shoudn't we change ISessionManager to receive a cancellation token also?

public interface ISessionManager
{
  Task<Session> RetrieveSession(TimeSpan timeout);
  Task<Session> RetrieveSession(CancellationToken ct);
  Session GetCachedSession();

  Task SendSession(TimeSpan timeout);
  Task SendSession(CancellationToken ct);
}

@StefanOssendorf
Copy link
Contributor Author

I woul say: no.
IIRC currently nowhere is a cancellation token supported. That would be a much greater change.
I think we should go with:
Change ISessionManager so RetrieveSession and SendSession now accept a TimeSpan timeout and populate it througout the calling chain to the top.

@luizfbicalho
Copy link
Contributor

I created with both options this way @StefanOssendorf can comment on my work and I can finish it the way will work best

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment