Closed emorell96 closed 2 years ago
JonPSmith
commented
2 years ago Developer shouldn't use the AuthPermissionsDbContext at all, but use the Roles, Tenant and AuthUser services. Also, cancelling a CancellationToken in EF Core throws an exception, which isn't a nice outcome - I try to return human-friendly errors.
For those reasons I don't think adding CancellationToken support is worth it.
emorell96
commented
2 years ago I meant adding CancellationToken support for the admin services (there's a bunch of Async methods on these services which are async but cannot be cancelled to save resources, say when you are getting a user or tenant through the admin services).
For the exception, the exception is a TaskCancelled exception which is the default exception when an exception is cancelled. This is a developer issue not a user issue if a Task is cancelled. So IMO it shouldn't be an IStatusGeneric but an exception that the developer can easily catch and decide what to do.
emorell96
commented
2 years ago For example methods like this: https://github.com/JonPSmith/AuthPermissions.AspNetCore/blob/ab4d37f30500707ea9d618b1927cabc305173ce2/AuthPermissions/AdminCode/IAuthUsersAdminService.cs#L29
They could benefit from a CancellationToken so that if a developer uses this, he can decide to cancel the operation after x seconds or simply when the data is no longer needed. And it shouldn't be a IStatusGeneric because then how do you simply catch when the operation is cancelled? With the exception is just a try{} catch(TaskCancelled ex) {} code block, and the developer has the choice of what to do. Either log it, or display an error or whatever. But with the StatusGeneric it's nice for when it's a problem shown to the user directly as you say on the wiki but it has a few downsides like not been able to localize the error messages if displayed to users, or doing all the things you'd do with a simple try catch block are now more difficult.
Btw, usually this is added by adding CancellationToken cancellationToken = default as the final parameter. This means that this is not a breaking change. If the cancellation token is not given nothing ever times out or is cancelled, but if he wishes to add the cancellationToken then there's an exception when the task is cancelled but that is the default expected behavior in all the c#, .NET 6 libraries/standard code. Deviating from this convention seems unwise.
JonPSmith
commented
2 years ago Hi @emorell96,
I have thought about this and I just don't think its worth it. All the methods access a normal database, not some microservice or HTTP api. Sorry.
If you want to do it then please have your own clone.
emorell96
commented
2 years ago Why would this only apply to an http api or a microservice? There's a reason the EF Core team uses cancelation tokens, it avoids having to waste database resources when the data is no longer needed, say the user navigates away before loading the data. This is a backend library and these features are c# features and best practices which allow the library to be used in high load scenarios as well.
I do not understand the reticence specially since it isn't a breaking change and something that I'd be happy to contribute even after you are done with the dev branch to make sure I do not work against what you are doing.
I will still have my own clone but it feels like a shame to have the code diverge on simple things like this :/
Get Outlook for Androidhttps://aka.ms/ghei36
From: Jon P Smith @.> Sent: Tuesday, June 7, 2022 8:00:14 PM To: JonPSmith/AuthPermissions.AspNetCore @.> Cc: Henry @.>; Mention @.> Subject: Re: [JonPSmith/AuthPermissions.AspNetCore] Adding CancellationToken support to async methods (Issue #44)
I have thought about this and I just don't think its worth it. All the methods access a normal database, not some microservice or HTTP api. Sorry.
If you want to do it then please have your own clone.
— Reply to this email directly, view it on GitHubhttps://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FJonPSmith%2FAuthPermissions.AspNetCore%2Fissues%2F44%23issuecomment-1148997207&data=05%7C01%7C%7C418dfbeca6e3424078b008da48af92ee%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637902216172864595%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vHlhAzVjEZoPachrGm0L5JgOvOVc7sT5PIAnpx0s1x0%3D&reserved=0, or unsubscribehttps://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAEZXY5ZCMOULJVVF2JSNAA3VN6E25ANCNFSM5YCJTHPQ&data=05%7C01%7C%7C418dfbeca6e3424078b008da48af92ee%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C637902216172864595%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=mG7yJm1BhhnHae6MGgbOBkW3lnW%2F3w0gNUVJY1RDXzA%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.***>
Hi,
I think it would be great if the library had CancellationToken support. After all, EF Core supports this already. It would just be a matter of forwarding the cancellation token to EF Core.