Feature/add cancellation token #3926
Conversation
…ion support Updated the `ISessionManager` interface and `SessionManager` class to include a `TimeSpan` parameter in the `RetrieveSession` and `SendSession` methods for timeout specification. Overloads of these methods have been added to accept a `CancellationToken` parameter for operation cancellation. The `StateManager` class's `GetState` and `SaveState` methods have also been updated to accept a `TimeSpan` parameter for timeout. Added a new project `Csla.Blazor.WebAssembly.Tests` for unit tests of the `SessionManager` class, including tests for timeout and cancellation scenarios. The solution file and a new project settings file have been updated accordingly. The `SessionManager` class now handles `OperationCanceledException` by rethrowing the exception.
Refactored `SessionManagerTests.cs` and `SessionManager.cs` to improve session handling. The `SessionMessage` object has been changed from a local variable to a class-level variable, allowing it to be accessed across different methods. The `RetrieveSession` and `SendSession` methods have been updated to return the session and assert that the returned session is equal to `SessionValue.Session`. The exception type expected in these methods with zero timeout and cancelled cancellation token has been changed from `TaskCanceledException` to `TimeoutException`. The `GetCancellationToken` method has been simplified to create a `CancellationTokenSource` with a timeout directly. In `StateManager.cs`, the `GetState` method has been updated to not return a `Session` object, instead, it just retrieves the session without assigning it to a variable.
This commit includes a significant refactoring of the `SessionManagerTests.cs` file. The `GetHttpClient` method has been simplified by removing the creation of a new `MemoryStream` instance and the use of `CslaBinaryWriter`. The stream's position is no longer reset to 0, and the array is directly obtained from the stream. Several test methods have been removed, including `RetrieveSession_WithTimeoutValue_ShouldNotThrowException`, `RetrieveSession_WithValidCancellationToken_ShouldNotThrowException`, and `SendSession_WithZeroTimeout_ShouldThrowTimeoutException`. These tests were checking for specific exceptions or session values. The `SendSession_WithTimeoutValue_ShouldNotThrowException` test method has been modified by removing the assertion that was previously checking if the operation is successful. Lastly, the `RetrieveCachedSessionSession` method has been modified by removing the call to `GetCachedSession` method of `_sessionManager`.
Updated `SessionManager.cs` methods `RetrieveSession` and `SendSession` to handle `TaskCanceledException` internally and rethrow as `TimeoutException`. Simplified `SendSession` by removing exception handling and refactored `RetrieveSession` to move `stateResult` handling outside of try-catch block. Renamed test methods in `SessionManagerTests.cs` to reflect these changes and updated expected exception type.
` ` `The SaveState method in StateManager.cs has been converted from a synchronous method to an asynchronous one. This is indicated by the addition of the async keyword and the change in return type from void to Task. Additionally, the call to _sessionManager.SendSession(timeout) within the SaveState method has been updated to use the await keyword, making this method call awaited, in line with the change to make SaveState an asynchronous method.`
Updated `Grpc.Net.Client` and `Grpc.Tools` packages in `Csla.Channels.Grpc.csproj`. Refactored `InitializeUser` method in `ApplicationContextManagerBlazor.cs` and `ApplicationContextManagerInMemory.cs` to be asynchronous. Removed `System.Transactions` reference from `Csla.Tests.csproj` and deleted `DataPortalTestDatabaseDataSet.Designer.cs` and related files. Added new test classes `HttpProxyExtensionsTests.cs` and `HttpProxyOptionsExtensionsTests.cs`. Removed `packages.config.old` file. Refactored `HttpProxy.cs` and `HttpProxyExtensions.cs` and added new property and methods in `HttpProxyOptions.cs`. Removed "Csla.Generators.CSharp" project from the solution "csla.test.sln" and its dependencies.
…error handling In this commit, several updates were made to the `SessionManager.cs` and `SessionManagerTests.cs` files. The variable `_sessionManager` was renamed to `_SessionManager` in `SessionManagerTests.cs`. The `Initialize` method was converted to an asynchronous method, and the `RetrieveSession` method call in it was updated to use `await`. XML comments were added to the `RetrieveSession`, `SendSession`, and `GetSession` methods in `SessionManager.cs` for better code documentation. The `RetrieveSession` and `SendSession` methods were updated to handle `TaskCanceledException` and throw a `TimeoutException` with a custom message. The `GetSession` method was updated to handle the case where `_session` is `null`, creating and returning a new `Session` object in this case. The `SendSession` method was updated to serialize the `_session` object and send it to the server if `SyncContextWithServer` is `true`. Finally, the `RetrieveSession` method was updated to retrieve the session from the server if `SyncContextWithServer` is `true`, deserializing and storing the retrieved session in `_session` or calling `GetSession` to get or create a new session if the retrieval is unsuccessful.
Refactored the `Initialize` method in `SessionManagerTests.cs` to be synchronous and removed the `RetrieveSession` call. The `RetrieveSession` call has been added to four test methods to ensure session retrieval before each test. Renamed and converted `RetrieveCAchedSessionSession` to an asynchronous method, adding a `RetrieveSession` call and an assertion for non-null cached sessions. Added a new test method `RetrieveNullCachedSessionSession` to assert null cached sessions.
Updated variable names `_SessionManager` and `SessionValue` to `_sessionManager` and `_sessionValue` respectively, to adhere to the common C# naming convention for private fields. All instances of these variables in the code, including in the `Initialize()`, `Deserialize()`, `RetrieveSession()`, `SendSession()`, and `GetCachedSession()` methods, as well as in test assertions, have been updated accordingly.
This commit represents a significant shift in the mocking framework used for unit testing in the `Csla.Blazor.WebAssembly.Tests.csproj` project. The `Moq` package has been replaced with `NSubstitute` in the project file and throughout the `SessionManagerTests.cs` file. This includes changes in the way mocks are created, set up, and how return values are specified for mocked methods and properties. Additionally, a new `TestHttpMessageHandler` class has been added to `SessionManagerTests.cs` to mock the behavior of an `HttpClient`. The `GetHttpClient` method has been updated to use this new class, aligning with the switch from `Moq` to `NSubstitute`.
Updated several files to enhance timeout handling and asynchronous operations. `SessionManager.cs` now includes exception handling for timeout scenarios in `RetrieveSession` and `SendSession` methods. `ViewModel.cs` has been updated for asynchronous saving of the Model with `CancellationToken` in `SaveAsync` method. `BusyHelper.cs` and `BusinessRules.cs` now include methods for waiting with timeout exceptions. `AsyncManualResetEvent.cs` now allows setting a `CancellationToken`. Added a new `TimeSpanExtensions.cs` file for converting `TimeSpan` to `CancellationToken`.
In this commit, CancellationToken support has been added across multiple classes and interfaces to allow for cancellation of various operations. This includes new methods in `ISessionManager`, overloads in `StateManager`, and updates to `BusinessRules`, `IDataPortalTarget`, and `BusyHelper`. Additionally, a new `WaitForIdle` method has been added to several classes and interfaces to allow for cancellation of idle waiting operations. Code in `ObjectFactory.cs` has been reformatted for better readability. No functionality changes have been made in the reformatting.
There was a problem hiding this comment.
I only checked the first four files (SessionManager, StateManager and ViewModel).
It seems all touched files have formatting issues. Because of that I abort the review until you recheck all files. The correct intendation is set by the editorconfig so this should be fixable by auto formatting.
This commit includes a series of changes aimed at improving code readability and organization across multiple files. Import statements have been rearranged in several files including `SessionManager.cs`, `BusinessBase.cs`, `BusinessListBase.cs`, and `CommandBase.cs`. Code blocks and methods in `SessionManager.cs`, `StateManager.cs`, `ViewModel.cs`, and `BusinessListBase.cs` have been reformatted for better readability, with no changes to the logic. The class constructor in `StateManager.cs` has been moved to the top for better visibility. `BusyHelper.cs` has seen a namespace change and addition of a new class with methods. Significant updates have been made to `DynamicBindingListBase.cs` including method updates, section additions and rearrangements. New lines have been added in several files for better readability. The overall structure of the code remains the same.
Refactored the `SaveState` method in `StateManager.cs`. Removed the calls to `SendSession` method of `_sessionManager` that were previously using `timeout` and `ct` parameters. The `SaveState` method no longer depends on `SendSession` method of `_sessionManager`. Overload InitializeAsync and refactor StateManager.cs Overloaded the `InitializeAsync()` method in `StateManager.cs` to accept a `CancellationToken` parameter and added a new private method `GetState(CancellationToken ct)`. This new method checks if the operating system is a browser and retrieves the session using the provided cancellation token. Removed the previous versions of `GetState(CancellationToken ct)` and `InitializeAsync(CancellationToken ct)` methods that were located in a different part of the code. Also, modified the `SaveState(CancellationToken ct)` method by removing the code block that sends the session using the provided cancellation token when the operating system is a browser.
I used the dotnet format csla.test.sln to resolve this issue, but there are too many changes across the solution, more than 300 files need change. I just updated those files changed in this PR, we can discuss later something to improve this. |
Yeah. Fixing the whole solution is out of scope. I'm only ever inspecting the files you changed. :) |
|
Regarding the change of timeout in async rule execution to use a CancelationToken - does this provide value? Right now it appears to be a different way to achieve the same result, and so the change brings on (probably minor) risk for no gain. On the other hand, if we think it is paving the way for a future where canceling async rule execution could be done via code, then perhaps there's value? |
Timeout I think that is much more limited, I have an example one project that one of my tasks didn't pass correctly the cancellation token, and this was a problem in te IIS reset, this task couldn't be cancelled in time, and used to break the IIS reset, causing 503 |
What I am getting at though, is that the code doesn't allow someone to cancel the operation because the cancelation token is created off the timeout and isn't exposed higher in the API. I think we need to consider what someone would want. If they call Inside an async rule, presumably we pass in the cancelation token (via |
|
I agree with Rocky here. We should remove the business rules changes here and make a proper design/concept for how we want to be able to use a cancellation token in the context of rules. |
In this commit, several changes have been made to the `BusinessRules.cs` file to improve code readability and efficiency. The `using` directives have been rearranged and unnecessary ones have been removed. The `CheckRulesAsync` and `WaitForAsyncRulesToComplete` methods have been modified to remove the conversion of `TimeSpan` to `CancellationToken`, and their overloads that accepted `CancellationToken` have been removed. The `CheckObjectRules` method now includes a new variable `oldRR` to store the value of `RunningRules` before it is set to `true`. Minor formatting changes have been made to the `CheckRules` and `CanRunRule` methods. The `CheckRulesForProperty` and `RunRules` methods have been refactored to improve the formatting of LINQ queries and remove unnecessary conditions. The assignment of `target` in `RunRules` has also been improved for better readability.
I changed it back to the original, and already solved the format problems, the tests I'll do in the weekend |
In NoOpSessionManager.cs, two new asynchronous methods `RetrieveSession(CancellationToken ct)` and `SendSession(CancellationToken ct)` have been added. These methods are versions of the existing `RetrieveSession` and `SendSession` methods that take a `CancellationToken` as a parameter. The `UpdateSession` method has also been modified to include a new parameter `newSession`. In ViewModel.cs, unnecessary using directives have been removed and a string literal in `SaveAsync` method has been replaced with `nameof(SaveAsync)` for better refactoring resilience. In BusyHelper.cs, a line of code in the `WaitForIdle` method has been modified, but the change does not seem to alter the functionality. This could be a mistake or an unnecessary change.
Added a new namespace `Csla.Test.Threading` and a new class `AsyncManualResetEventTests` in the `AsyncManualResetEventTests.cs` file. This class contains several unit tests for the `AsyncManualResetEvent` class. The tests cover the behavior of the `SetCancellationToken` and `WaitAsync` methods, checking task cancellation based on the cancellation token status and whether the task waits until the reset event is set. These tests are written using the `Microsoft.VisualStudio.TestTools.UnitTesting` framework.
|
Resolve the confligt and we are ready to merge :) |
Are you sure? I'm still making some more tests |
Updated `Csla`, `Csla.AspNetCore`, and `Csla.Blazor` packages from `8.0.0-R24021201` to `9.0.0-R24051102` across multiple projects. Refactored `@rendermode` in `Counter.razor` and `EditPerson.razor` and updated `IncrementCount` function. Removed `IsStateReady` variable from `Home.razor` and `Get` and `Put` methods from `CslaStateController.cs`. Updated `AddServerSideBlazor` and `AddBlazorWebAssembly` methods in `Program.cs` and `ConfigurationExtensions.cs` respectively. Added `SaveState` method in `StateManager.cs`. Updated lambda expressions and `using` statements in multiple files to improve readability and efficiency.
Removed the `timeout` parameter from the `SaveState` method in `StateManager.cs`. The method signature has been updated from `public async Task SaveState(TimeSpan timeout)` to `public async Task SaveState()`. The `timeout` parameter was previously used in the `_sessionManager.SendSession(timeout)` method call, but this has been removed and the method is now called without any arguments as `_sessionManager.SendSession()`. The XML comment for the `timeout` parameter has also been removed to reflect the change in the method signature. This change suggests that the `SendSession` method might have been updated to no longer require a `timeout` parameter, or the timeout is being handled differently now.
I won't say no to more useful tests :) |
Updated the `ViewModelSaveAsyncErrorTests.cs` file to include the `Csla.Core` namespace. Added two new test methods: `SavingWithCancellationToken_Success` and `SavingWithCancelledCancellationToken_ErrorEventIsInvoked` to test ViewModel saving with a valid and cancelled CancellationToken respectively. Also, introduced a new private class `FakeBusy` that implements the `ITrackStatus` interface from the `Csla.Core` namespace for tracking object status.
This commit includes a major cleanup of unnecessary namespaces across multiple files and updates exception handling to use `nameof` instead of string literals for parameter names. The `System.Collections.Immutable` and `System.Linq` namespaces were removed from `TestHelpers.cs` and `ApplicationContextManagerInMemory.cs` respectively. Several namespaces were removed from `SessionManagerTests.cs`, `SessionManager.cs`, `TestResults.cs`, `Controller.cs`, `HasPermissionAttribute.cs`, `ViewModelBase.cs`, `ErrorEventArgs.cs`, and `OutputCoercionTest.cs`. The `System.Reflection.Emit` namespace was added to `DynamicMethodHandlerFactory.cs` under certain conditions and the `System.IO` and `System.Runtime.Serialization.Formatters.Binary` namespaces were added to `BinaryFormatterWrapper.cs`. The `System.ArgumentException`, `System.ArgumentOutOfRangeException`, and `System.ArgumentNullException` in multiple files were updated to use `nameof` for parameter names.
Added a new test class `WaitForIdleTests` in the `Csla.Server.Tests` namespace with four test methods to validate the behavior of the `WaitForIdle` method under different conditions. Introduced two new classes `FakeBusinessBase` and `FakeEntity` for testing purposes. `FakeEntity` implements several interfaces and includes a `IsBusy` property used in the tests. Unimplemented methods and properties in `FakeEntity` throw a `NotImplementedException`. Both `WaitForIdleTests` and `FakeEntity` use the `Csla.Core`, `Csla.Serialization.Mobile`, and `Microsoft.VisualStudio.TestTools.UnitTesting` namespaces.
I added some more. Please check if it's all ok |
Updated and refactored several test methods in `ViewModelSaveAsyncErrorTests.cs` and `WaitForIdleTests.cs`. Removed unnecessary `Task.Delay` and `return person` lines from `SavingWithCancelledCancellationToken_ErrorEventIsInvoked` method. Added new constructor and methods to `FakeBusy` class. Removed `using Csla.Core` and `using Csla.Serialization.Mobile` directives and added `using FluentAssertions` directive in `WaitForIdleTests.cs`. Replaced `Assert.IsTrue(true)` with `target.IsBusy.Should().BeFalse()` in `WaitForIdle_Should_WaitForIdle` and `WaitForIdle_Should_WaitForIdleWithCancellation` methods. Updated `WaitForIdle_Should_ThrowCancelledTaskException` method by removing `FakeBusinessBase` and `FakeEntity` classes. Added new assertions in `WaitAsync_ShouldWaitUntilSet` method in `AsyncManualResetEventTests.cs`. Moved `FakeBusinessBase` and `FakeEntity` classes to new files and updated `FakeEntity` class with new event handlers and methods.
Significant refactoring of the `FakeEntity` class in `FakeEntity.cs` has been done. Event handlers `BusyChanged`, `UnhandledAsyncException`, and `Saved` have been added. `NotImplementedException` has been removed from several properties and methods, which now have getters, setters, empty bodies, or return default values. The `Save` methods return an empty string, `SaveAndMergeAsync` methods return a completed task, and `SaveAsync` methods return a task with a new object. The `SetParent` method remains unchanged.
Refactored `FakeBusy` and `FakeEntity` classes in `ViewModelSaveAsyncErrorTests.cs` and `FakeEntity.cs` respectively. Removed event handlers for `BusyChanged` and `UnhandledAsyncException` from constructors and deleted corresponding private methods. Updated `IsBusy` property to a full property with a private backing field `_IsBusy` and modified its setter and getter to invoke `BusyChanged` and `UnhandledAsyncException` events respectively. Enhanced `Save` method in `FakeEntity.cs` to invoke `Saved` event and wrapped `SaveAndMergeAsync` method in a try-catch block to handle exceptions. Added a new test method `WaitForIdle_Should_ThrowCancelledTaskException` in `WaitForIdleTests.cs`.
Add cancellation token as an alternative to timeout
closes #3911