Raise exceptions from callbacks on SynchronizationContext

[aud0201]

If completeTask ouccred an exception, throws an exception using GetAwatier().OnCompleted().

424

[aud0201]

@dotnet-policy-service agree

[Daniel-Svensson]

Hi @aud0201

First thank you for making your first contribution, it is very appreciated.

Some review comments

1. I am not 100% convinced that "OnCompleted" will make the exception appear on the dispatcher (I had excepted that using CurrentSynchronizationContextTaskScheduler to do so but it did not) It may be better to be exclicit with invoking the code on the syncronization context
   • By rethrowing the exception in OnCompleted you will rewrite the callstack , loosing the original information which can be avoided by using ExceptionDispatchInfo

I had a quick lock at this and came up with a slightly different solution. You can have a look at (feel free to take the code) https://github.com/OpenRIAServices/OpenRiaServices/compare/main...Daniel-Svensson:OpenRiaServices:throw_exceptions_on_syncContext

• Maybe the code I submitter is better placed in OperationBase.InvokeCompleteCallbacks to make testing easier (in that case you might be able to modify the test above) ? https://github.com/OpenRIAServices/OpenRiaServices/blob/5ff0fac89fb77c1e3e57117bbd96e116d13a6f3c/src/OpenRiaServices.Client/Framework/OperationBase.cs#L342-L370

2. Do you know if the authentication logic has the same problem, it might also use FromCurrentSynchronizationContext`?

   • I found one usage in AuthenticationService.StartOperation

3. Would you be interested to also write an unit test ?

I belive a good test should

1. Create a custom/derived SyncronizatonContext class so that can record if exceptions are thrown from the Send method
2. Call Load or CompleteAsync with a callback which throws an exception (and maybe a separate test which just don't handle the exception?)
3. Verify that an exception was thrown on the SyncronizatonContext as expected

There are some tests for similar cases in OperationTests such as

• UnhandledInvokeOperationError
• UnhandledLoadOperationError
• UnhandledSubmitOperationError

https://github.com/OpenRIAServices/OpenRiaServices/blob/5ff0fac89fb77c1e3e57117bbd96e116d13a6f3c/src/OpenRiaServices.Client/Test/Client.Test/Data/OperationTests.cs#LL121-L245

[aud0201]

@Daniel-Svensson

Thank you for your response. I will try implementing it based on the guidance you provided.

[aud0201]

hi @Daniel-Svensson

Based on your suggestions, I have modified OperationBase.InvokeCompleteCallbacks and created a unit test.

As for unit tests, I haven't written many, so I'm not sure if I need to write more tests.

[Daniel-Svensson]

God job so far, I think this makes a god contribution and should make it into a new release without to much delay (probably 5.4.0)

As for unit tests, I haven't written many, so I'm not sure if I need to write more tests.

I will se if @SandstromErik has the time to review and think about tests.

Without thinking much about it it might be a good idea to update the 3 test methods

• UnhandledInvokeOperationError
• UnhandledLoadOperationError
• UnhandledSubmitOperationError

So that they also check that that the exception is thown on synronization context instead of the current approach

[Daniel-Svensson]

/azp run

[azure-pipelines[bot]]

No commit pushedDate could be found for PR 427 in repo OpenRIAServices/OpenRiaServices

[Daniel-Svensson]

/azp run /azp help

[azure-pipelines[bot]]

Command 'run /azp' is not supported by Azure Pipelines.

Supported commands

• help:
• Get descriptions, examples and documentation about supported commands
• Example: help "command_name"
• list:
• List all pipelines for this repository using a comment.
• Example: "list"
• run:
• Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
• Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
• where:
• Report back the Azure DevOps orgs that are related to this repository and org
• Example: "where"

See additional documentation.

[Daniel-Svensson]

/azp run

[azure-pipelines[bot]]

No commit pushedDate could be found for PR 427 in repo OpenRIAServices/OpenRiaServices

[Daniel-Svensson]

@aud0201 I ran some additional tests on this (I tried to rewrite the test for "normal" unhandled exceptions to verify they work correctly with SyncronizationContext) and discovered that may proposed method did not handle that case).

I've fixed that scenario and updated those tests as well as added a note to the changelog.

If everything builds the changes should be merged soon. Thank you for your contribution

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

[Daniel-Svensson]

Released to nuget as preview.2 , @aud0201 please try it out

[aud0201]

It works! @Daniel-Svensson

This update solved a lot of headaches in my project migrating

I've learned a lot from this. thank you very much 👍