haf / expecto

A smooth testing lib for F#. APIs made for humans! Strong testing methodologies for everyone!
Apache License 2.0
663 stars 96 forks source link

Potential deadlock in test/async interaction #406

Closed pbiggar closed 3 years ago

pbiggar commented 3 years ago

After disabling the spinner in #405, I'm still getting some occasional hangs. I disabled most of my test code and gradually re-enabled it, finally pointing to the source: my tests fail when I call System.IO.File.ReadAllBytesAsync. That seems like a function that shouldn't cause problems.

I've recently been reading through https://medium.com/rubrikkgroup/understanding-async-avoiding-deadlocks-e41f8f2c6f5d, which is a really excellent resource to understand deadlocks. One of the issues it raises is blocking on tasks from other tasks.

I looked at the expecto code and found something which seems suspicious in the testTask CE:

  let a = async {
          do! task.Run f |> Async.AwaitTask
      }

I think this shouldn't cause deadlock, but the article led me to believe that something like this is pretty fishy, even though I can't articulate what could go wrong here. I would imagine you can do async { Async.AwaitTask f } without the intermediate task.Run, instead.

I'm going to try this out to see if it fixes the issue I have. I don't know if this is actually a problem but I wanted to flag it for your attention in case I get busy and wander off.

Thanks!

haf commented 3 years ago

@pbiggar Thank you for looking into it. While not all bugs are shallow with more eyes, you're steaming off a bit of the sea ;)

pbiggar commented 3 years ago

Looks like I was mistaken. I thought the code said Task.Run but it says task.Run. The lowercase task is the task CE copied from TaskBuilder, which the TestTaskBuilder is copied from, and not the Task module.