ramondeklein
add CancellationToken
- add CancellationToken to IHttpContext
- pass cancellationToken to async Framework methods
This is a breaking change
I think it's a good idea to add a CancellationToken, but I would use a more traditional method by passing the CancellationToken to the handlers. I have made a start with this in the add-cancellationtoken branch.
We should also pass the cancellation token to the IStore, IPropertyManager and ILockingManager to make sure these operations are also cancelled. I haven't implemented this yet, but this can be done quite easily (I guess).
But there is one important issue and that is signalling that a request has been canceled. Neither ASP.NET, HttpListeren or the Kestrel versions raise an event that the request has been aborted (or I have missed that part). If there is no way to signal that the request has been aborted, then it doesn't make much sense to pass the cancellation token around (if it is always CancellationToken.None and never set).
PS: I don't mind a breaking change if it makes NWebDAV more stable. Adding a cancellation token should be pretty straightforward for most developers.
I first added a CancellationTokento all interface methods, but then thought it would be more convinient to use the existing request wrapper. If we are both on the same side that async methods should have a CancellationTokenParameter, I will modify my branch/pr and just pass the CancellationTokenwherever needed.
I've seen kestrel (aspnetcore2.0) and ASP.NET (net461) signalling an aborted request in MVC and Razor Pages - that's why I started this PR. Kestrel uses the HttpContext.RequestAborted Token and ASP.NET even has two - HttpContext.Response.ClientDisconnectedToken and HttpContext.Request.TimedOutToken.
I will take a look at your branch, and update my pr accordingly.
Great! Both frameworks have an abort/timeout token, so it’s a good idea to allow cancellation. It should also be propagated to the underlying stores and managers. NWebDAV probably spends most of its time there. Passing the cancellation token around may seem a burden, but it clearly shows that cancellation is supported.
I have updated my branch and propagated the CancellationToken through the stores, property manager and locking manager. I also removed the CancellationToken from the request, but pass it to the dispatcher directly. This also allows other scenario's for cancelling requests.
I need to do some checking to determine if this doesn't add to much overhead (making operations async doesn't come for free), but I think it shouldn't make a big difference in this scenario. It also allows for locking managers that rely on async behaviour (i.e. locking manager using Redis, ...).