New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore Failure on Mono: System.Exception: Expected an result at this place. #2639

Closed
0x53A opened this Issue Aug 20, 2017 · 11 comments

Comments

Projects
None yet
2 participants
@0x53A
Contributor

0x53A commented Aug 20, 2017

I've been skimming through the travis logs and found one error that looked ... interesting:

image

https://travis-ci.org/fsprojects/Paket/jobs/266475236#L1953

It comes from this line:

let internal memoizeAsyncEx (f: 'iext -> 'i -> Async<'o * 'oext>) =
let cache = System.Collections.Concurrent.ConcurrentDictionary<'i, System.Threading.Tasks.Task<'o>>()
let handle (ex:'iext) (x:'i) : Choice<System.Threading.Tasks.Task<'o>, Async<'o * 'oext>> =
//fun (ex:'iext) (x: 'i) -> // task.Result serialization to sync after done.
let mutable result = None
let mutable wasAdded = false
let task = cache.GetOrAdd(x, fun x ->
wasAdded <- true
async {
let! o, oext = f ex x
result <- Some oext
return o
} |> Async.StartAsTask) // |> Async.AwaitTask
if not wasAdded then
Choice1Of2 task
else
async {
let! res = task |> Async.AwaitTask
match result with
| Some ext -> return res, ext
| None -> return failwithf "Expected an result at this place."

Just judging from the context, it seems like this should not happen and represents either a mono async issue, or an actual bug. But I haven't analyzed it in any way.

If you don't care about this, just close the issue ;-)

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 20, 2017

Member

@0x53A Yeah I have no idea how it is possible to reach this code-place, that's the reason why I added the bool variable after all.

If the bool is true the result should be set after the task finishes (which it is after the task is finished).
The assumption is wrong when f throws but then the match is not reached.

Yeah let's look at this closely and leave this open for a while.

Member

matthid commented Aug 20, 2017

@0x53A Yeah I have no idea how it is possible to reach this code-place, that's the reason why I added the bool variable after all.

If the bool is true the result should be set after the task finishes (which it is after the task is finished).
The assumption is wrong when f throws but then the match is not reached.

Yeah let's look at this closely and leave this open for a while.

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Aug 20, 2017

Contributor

Hm, I don't think simple variables are guaranteed to be consistent across Threads.
This may need a 'volatile' or something different.

Edit: this sets wasAdded <- true, but then starts a background task, which sets result <- Some oext, so if it reads in-between it can read true / None. If the system is cpu-limited, the task may be executed at an unspecified point in time.

Contributor

0x53A commented Aug 20, 2017

Hm, I don't think simple variables are guaranteed to be consistent across Threads.
This may need a 'volatile' or something different.

Edit: this sets wasAdded <- true, but then starts a background task, which sets result <- Some oext, so if it reads in-between it can read true / None. If the system is cpu-limited, the task may be executed at an unspecified point in time.

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 20, 2017

Member

Tbh I'd expect the variable to be read from the same thread, but whatever

Member

matthid commented Aug 20, 2017

Tbh I'd expect the variable to be read from the same thread, but whatever

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Aug 20, 2017

Contributor

Thread 1 sets wasAdded <- true, and then schedules Async.StartAsTask.
Thread 2 (ThreadPool) then at an unspecified time later sets result <- Some oext

Contributor

0x53A commented Aug 20, 2017

Thread 1 sets wasAdded <- true, and then schedules Async.StartAsTask.
Thread 2 (ThreadPool) then at an unspecified time later sets result <- Some oext

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 20, 2017

Member

Ah fuck, thanks :)

Member

matthid commented Aug 20, 2017

Ah fuck, thanks :)

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 20, 2017

Member

Was only thinking about the bool variable. Ok then we could make the result part of the task and map the task later. The bad thing about this is than this unused value is in the cache :/.
I think there will be two tasks, one for the cache and one we set into the variable (instead of the bool)

Member

matthid commented Aug 20, 2017

Was only thinking about the bool variable. Ok then we could make the result part of the task and map the task later. The bad thing about this is than this unused value is in the cache :/.
I think there will be two tasks, one for the cache and one we set into the variable (instead of the bool)

@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Aug 20, 2017

Contributor

I don't really understand the code, and why do you have the Choice?

As I understand it, the first call gets Choice1, subsequent calls get Choice2? That's not really memoize anymore ;-)

The simplest code, imo, would be:

let internal memoizeAsyncEx (f: 'iext -> 'i -> Async<'o * 'oext>) =
    let cache = System.Collections.Concurrent.ConcurrentDictionary<'i, System.Threading.Tasks.Task<'o>>()
    let handle (ex:'iext) (x:'i) : Choice<System.Threading.Tasks.Task<'o>, Async<'o * 'oext>> =
        let task = cache.GetOrAdd(x, fun x ->
            let tcs = TaskCompletionSource()
            
            Async.Start (async {
                try
                    let! o, oext = f ex x
                    tcs.SetResult o
                with
                  exn ->
                    tcs.SetException exn
            })

            tcs.Task)
        Choice1Of2 task
    handle

That puts a Task from a TCS into the dictionary. The async call runs in the background in the threadpool and sets the TCS when it finishes / fails.

But this would lose the distinction between first call / later calls

Contributor

0x53A commented Aug 20, 2017

I don't really understand the code, and why do you have the Choice?

As I understand it, the first call gets Choice1, subsequent calls get Choice2? That's not really memoize anymore ;-)

The simplest code, imo, would be:

let internal memoizeAsyncEx (f: 'iext -> 'i -> Async<'o * 'oext>) =
    let cache = System.Collections.Concurrent.ConcurrentDictionary<'i, System.Threading.Tasks.Task<'o>>()
    let handle (ex:'iext) (x:'i) : Choice<System.Threading.Tasks.Task<'o>, Async<'o * 'oext>> =
        let task = cache.GetOrAdd(x, fun x ->
            let tcs = TaskCompletionSource()
            
            Async.Start (async {
                try
                    let! o, oext = f ex x
                    tcs.SetResult o
                with
                  exn ->
                    tcs.SetException exn
            })

            tcs.Task)
        Choice1Of2 task
    handle

That puts a Task from a TCS into the dictionary. The async call runs in the background in the threadpool and sets the TCS when it finishes / fails.

But this would lose the distinction between first call / later calls

0x53A added a commit to 0x53A/Paket that referenced this issue Aug 20, 2017

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 20, 2017

Member

@0x53A Yes this is a bit strange and I have no idea how to better encode it.
But the general idea is to have an async call which returns some generally cache-able results but also some data which is not cache-able but re-usable from the caller.
It's basically used for the blacklisting logic:

  • The thing to cache is if the URL is good
  • The thing to use one-time only is the actual result
  • We don't want to start a second request if we don't know jet if the URL is good
  • We don't want to make an additional request the first time.
Member

matthid commented Aug 20, 2017

@0x53A Yes this is a bit strange and I have no idea how to better encode it.
But the general idea is to have an async call which returns some generally cache-able results but also some data which is not cache-able but re-usable from the caller.
It's basically used for the blacklisting logic:

  • The thing to cache is if the URL is good
  • The thing to use one-time only is the actual result
  • We don't want to start a second request if we don't know jet if the URL is good
  • We don't want to make an additional request the first time.
@0x53A

This comment has been minimized.

Show comment
Hide comment
@0x53A

0x53A Aug 20, 2017

Contributor

Ah, ok, so you do a request, and key it by the source.

So it gets blacklisted if the first request fails, but stays non-blacklisted, if the first succeeds, but the second one fails, right?

Please, please, create a custom DU for that stuff and don't abuse the poor Choice ;)

Contributor

0x53A commented Aug 20, 2017

Ah, ok, so you do a request, and key it by the source.

So it gets blacklisted if the first request fails, but stays non-blacklisted, if the first succeeds, but the second one fails, right?

Please, please, create a custom DU for that stuff and don't abuse the poor Choice ;)

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 20, 2017

Member

Like I said I'm open for suggestions. I'm not sure if DU is the problem in this particular snippet :). But yeah a good idea!

Member

matthid commented Aug 20, 2017

Like I said I'm open for suggestions. I'm not sure if DU is the problem in this particular snippet :). But yeah a good idea!

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Aug 20, 2017

Member

So it gets blacklisted if the first request fails, but stays non-blacklisted, if the first succeeds, but the second one fails, right?

Yeah basically, however it seems like this is not always working as expected. I saw urls being blacklisted later and paket continuing it's job. Maybe I have failed somewhere.

Member

matthid commented Aug 20, 2017

So it gets blacklisted if the first request fails, but stays non-blacklisted, if the first succeeds, but the second one fails, right?

Yeah basically, however it seems like this is not always working as expected. I saw urls being blacklisted later and paket continuing it's job. Maybe I have failed somewhere.

0x53A added a commit to 0x53A/Paket that referenced this issue Aug 21, 2017

@forki forki closed this in #2641 Aug 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment