AsyncLock dispose deadlock #57
Comments
|
I'm not following you. Can you post a unit test or gist code sample that demonstrates the deadlock? |
|
I will try but its proving difficult to reproduce outside the system the behavior is observed in. I was incorrect about where the problem is. It is the when disposing of the result of Looks like the deadlock happens when disposing of the CompletionDisposable. It tries to set the result of the task completion source. Stack trace to the block.
|
|
Interesting. I have a deadlock (or at least lock build-ups that last a looong time) issue that occurs sporadically at one client site and have been adding a lot of logging to try to track down the source but so far haven't had a lot of success, and they're upending their IT right now so I'm hoping it's more their environment and bad database server causing delays. |
|
@IanYates Excellent idea. I actually had a build of AsyncEx with exhaustive ETW tracing for all synchronization primitive operations. Unfortunately, it proved too difficult to make portable in a reasonable fashion. FYI, there is an |
|
Hi Stephen, |
|
@chanti-github Can you provide code that reproduces the problem? In the meantime, I suspect that upgrading to the |
|
Sorry this issue is difficult to reproduce i am using the follwoing way using (await Constants._lock.LockAsync ()) { |
|
@StephenCleary can you please update on this ?? |
|
@chanti-github Have you tried upgrading to the new version, as I suggested? Also, if you could develop a minimal, complete, verifiable example, that would enable me to track down the root cause. |
|
Do you want me update Nito.AsyncEx Pacakage ?? |
|
@chanti-github Try removing Nito.AsyncEx and installing the prerelease version of Nito.AsyncEx.Coordination. |
|
you are saying it is pre release version will i get any issues ?? |
|
@chanti-github - well you have deadlocks (or at least lock build-up) at the moment. It can't really get much worse than that. |
|
I have an example that I believe will reproduce the issue. It is occurring when one task uses Lock and another uses LockAsync. The task using the Lock method does not terminate the dispose. If both tasks use Lock or LockAsync exclusively the problem does not occur. It is important that first task not await the task delay. If task1 awaits thus releasing the thread then task2 will complete. |
|
Any Updates Please ?? |
|
@chanti-github Have you tried upgrading to the new version, as I suggested? |
|
No, I will try today and let you know but i am able to see the issue several times |
|
I am not able to add package it is showing this error "Could not install package 'Nito.AsyncEx.Coordination 1.0.0-delta-7'. You are trying to install this package into a project that targets 'portable-net45+win+wp80+MonoAndroid10+xamarinios10+MonoTouch10', but the package does not contain any assembly references or content files that are compatible with that framework. For more information, contact the package author." |
|
i am not able to add package to my solution (mvvmcross) |
|
|
|
thanks for updates on package can you please let us know when you will move
|
|
@StephenCleary |
|
I've pulled down the source, made the change and, as of yesterday, am using the one-line change in production. So far so good. (this was less of an upheaval, and I was happier to do the quick production test, than if I switched over to the new AsyncEx package but I still plan to jump ship to it in the coming weeks) |
|
@sarathdev @chanti-github I will update AsyncEx as soon as possible. Unfortunately, I do not have the capability to build it at the moment, and it requires multiple specific versions of VS and other tools. Building the build machine will take a couple days of work, and I'm not sure when I'll have that time.\ (My old build machine was my old laptop, which has died. My new build machine will be a VM, so this will hopefully be the last time I'll have to build a build machine). |
|
That explains the build.ps1 file - I can see why you're having trouble :) I couldn't run it either as I only have VS 2015 on my current machine. |
|
Have you considered just using AppVeyor? |
|
@ejsmith I'm not sure if AppVeyor has a build machine that would suffice. My new (netcore) AsyncEx libraries all use AppVeyor, but they only target new platforms. The legacy AsyncEx requires some extremely legacy tooling. In particular, a modern system (VS2013+) in addition to VS2012 with the SL4 and WP71 SDKs. I made three separate attempts last week to try to set up a build VM. All of them failed, most notably due to the WP71 SDK. I am planning to make two additional attempts - I think it might work if I start with Win7, uninstall all the .NET Framework components, and build it up starting with VS2010 and the legacy SDKs, and then add VS2012 and VS2013 from there. If that doesn't work, I'll try the same thing using Vista (!) as a starting point. If that doesn't work, then we're screwed. The only real option forward at that point would be to drop support for SL4 and WP71. Which would considerably reduce the build machine setup complexity. |
|
That seems like a pretty reasonable thing to me. :-) Both of those platforms are no longer supported. This is an OSS project you do in your spare time. I think it's pretty reasonable to not want to spend your spare time trying to support legacy platforms especially if they add a lot of effort. Also, people are free to continue using the older packages if they want. |
|
@ejsmith Well, I know a good number of AsyncEx consumers are on unsupported platforms - most notably .NET 4. I also know there are SL and WP consumers, though I'm not sure if SL4 and WP71 are necessary - unfortunately, there is no way to track this. I think dropping SL4/WP71 makes sense, but we'd need to keep net4/net45/sl5/wp80/wpa81/win8 targets. The new AsyncEx does not support any of them. :/ |
|
Ahh... I didn't realize that this was the legacy repo. :-) |
|
I am using httpclient single instance approach but i am getting the task was canclled exception in times can you please help me on this ?? |
|
@chanti-github I don't believe that has anything to do with this issue; please post a question to Stack Overflow. |
|
@StephenCleary i saw so many questions related to this so i did not get any answers with curiosity i asked. |
|
FYI, the suggested fix is still working well for me. |
|
Should be fixed in |
|
This problem has hit us in production, with Nito.AsyncEx 4.0.1.0. We were using only When load suddenly spiked, the number of threads started rising and rising, and certain requests weren't being returned. It eventually reached thousands of threads. We took a dump, from it we could see the problem. Most of the threads (thousands) were waiting on And a single thread was stuck here: After we switch to the regular Unfortunately we don't have a repro to share. Is |
|
Short answer: yes, |
|
We have a theory about why this happened. It seems due to a spike in traffic or a hot start of a server, many requests come in all at once, each represented by a delegate task enqueued to the thread pool. Unfortunately, they all require a single dependency (or "shared resource" to use your term) that needs to be initialized first. This initialization takes about a second, during which tens of thousands of requests have queued up in the thread pool. Near the end of the initialization, we have a small, trivial piece of code (nothing blocking) that is protected by an AsyncLock. Only the first thread that reaches it enters it, the rest are waiting in a queue. If the initialization of the shared resource has already been performed, it is skipped, but that part with When the lock is released via Granted, we shouldn't have used Does the |
|
Here's a simple repro: static AsyncLock l = new AsyncLock();
void Main()
{
Console.WriteLine("Starting tasks...");
var tasks = Enumerable.Range(0, 100).Select(i => Task.Run(() => Do1(i))).ToArray();
Console.WriteLine("Tasks started. Waiting for all to complete...");
Task.WaitAll(tasks);
Console.WriteLine("All done!");
}
void Do1(int i)
{
if (i == 0)
Thread.Sleep(100);
using (l.Lock())
{
// mutate shared state atomically
}
Console.WriteLine("Done: " + i);
}
void Do2(int i)
{
if (i == 0)
Thread.Sleep(100);
lock (l)
{
// mutate shared state atomically
}
Console.WriteLine("Done: " + i);
}If you have |
|
Thanks for the repro. I've confirmed that the problem does not occur when using However, asynchronous coordination primitives - by their nature - are sensitive to thread pool exhaustion in some scenarios. If you can use |
|
The repro provided by @Allon-Guralnek will cause the issue for me. In LinqPad I've got this You'll note that I've got a Task.Run() in the main() method to get it started. Sometimes it runs through just fine. Other times it'll stop. Example output and 1 minute 13 seconds later I'm still nowhere :( But I can hit stop & start and get it working again. 9 times out of 10 it works properly. Removing the Console.WriteLine inside the loop seems to make it fail more often. With LinqPad I can quickly swap between So this appears to be a regression where v3.x - with the one-line patch from earlier in this issue - is good but AsyncEx v4.x and v5.x are both bad. It's a tricky one as it's intermittent and does eventually come good. |
I am running into a dead lock when using the AsyncLock. The situation as best as I can determine occurs when an AsyncLock.Lock() is being disposed. The task.WaitAndUnwrapException() call uses an awaiter that releases the current thread. Another tasks then grabs this thread. Until the second task complete or awaits the AsyncLock cannot complete because it requires the original calling context.
I noticed that the overload of WaitAndUnwrapException does not use the GetAwaiter method so it never gives up control of the thread. Would a simple fix be to use this overload in the AsyncLock?
The text was updated successfully, but these errors were encountered: