dotnet
Remove nonreentrantlock type
This type was created and used in a time that predated both TPL and helper types like SemaphoreSlim. We can remove this and just use what the platform gives us for this basic concept.
I would prefer not changing to object as it then requires ensuring that all callsites add throws that they don't already have teh lock. It would be easy to miss, and then end up accidentally reentering.
I'm not very concerned about deadlocking. If things are broken, having it deadlock is just as broken. Practically speaking, we haven't even hit this exception well... ever. So going through contortions to try to preserve it seems unnecessary. We have tons of code using semaphoreslim effectively, and it was never a problem that that code would similarly deadlock, so we should just keep things in that form for the rest of our code.
@jasonmalinowski @sharwell plz :) i think the gains made by removing this superfluous type and unifying on a common pattern we use in vastly more places more than outweigh speculative benefits around error conditions that we haven't ever actually benefitted from :)
At this point, I don't have any particular concerns about the type. It seems self-contained, simple, and clear enough to work for the cases where it's being used. My recommendation is to leave it as-is, and later when the compiler is updated to support custom lock functionality modify it at that time to be compatible with the new feature.
I would be amenable to alterations to the implementation which bring this type closer in line to the API of System.Threading.Lock, e.g. renaming IsOwnedByMe to IsHeldByCurrentThread.
This is just a personal recommendation. I won't request escalation to a managerial block in the event other members disagree with this view.
I'm not strongly against a long-term removal of this but I do want to correct this one statement:
Practically speaking, we haven't even hit this exception well... ever.
Right now there's a bug against me where we are hitting this and this PR is reminding me I forgot to fix that for 17.10 Preview 1. And a very quick Kusto search implies we may have at least two other bugs too where we'd be replacing an exception with a hang. There's an internal outage of some tools right now I'd use to file bugs so I'll follow up on those others when it's back, but in the mean time I'd least request that we don't merge this until those fixes go in, or we better understand those other ones to deem them not a significant issue.
But it does make me hope that if we are going ahead with the new C# lock feature that the framework would have a non-reentrant compatible lock with it and then we could ditch this code and just use that.
but in the mean time I'd least request that we don't merge this until those fixes go in,
I can deal with that.
@jasonmalinowski -- Can you share the query with how these exceptions are being found? Is it possible that we are under-reporting the number of times this occurs by either blanket catching or losing the exception otherwise?
