Skip to content
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

An async task that picks up a patch for a mocked method can MethodError on world age issue #108

Closed
NHDaly opened this issue May 8, 2023 · 1 comment · Fixed by #109
Closed

Comments

@NHDaly
Copy link
Member

NHDaly commented May 8, 2023

This is very similar to #89, but a slight variant:

If an async task was scheduled before a patch was created, and it picks up the patch during a patch() do block, then it will throw a MethodError because the patch is too new:

julia> using Mocking

julia> function foo(x)
           @mock bar(x)
       end
foo (generic function with 1 method)

julia> bar(x) = x
bar (generic function with 1 method)

julia> Mocking.enable()

julia> ch = Channel() do ch
           put!(ch, foo(take!(ch)))
       end
Channel{Any}(0) (empty)

julia> p = @patch bar(x) = 10*x
Patch{typeof(bar)}(bar, var"##bar_patch#314")

julia> apply(p) do
           put!(ch, 2)
           @info take!(ch)
       end
┌ Error: Exception while generating log record in module Main at REPL[7]:3
│   exception =
│    TaskFailedException
│    Stacktrace:
│      [1] try_yieldto(undo::typeof(Base.ensure_rescheduled))
│        @ Base ./task.jl:871
│      [2] wait()
│        @ Base ./task.jl:931
│      [3] wait(c::Base.GenericCondition{ReentrantLock})
│        @ Base ./condition.jl:124
│      [4] take_unbuffered(c::Channel{Any})
│        @ Base ./channels.jl:433
│      [5] take!(c::Channel{Any})
│        @ Base ./channels.jl:410
│      [6] macro expansion
│        @ ./logging.jl:360 [inlined]
│      [7] (::var"#3#4")()
│        @ Main ./REPL[7]:3
│      [8] apply(body::var"#3#4", pe::Mocking.PatchEnv)
│        @ Mocking ~/.julia/packages/Mocking/MRkF3/src/patch.jl:132
│      [9] apply(body::Function, patches::Patch{typeof(bar)}; debug::Bool)
│        @ Mocking ~/.julia/packages/Mocking/MRkF3/src/patch.jl:139
│     [10] apply(body::Function, patches::Patch{typeof(bar)})
│        @ Mocking ~/.julia/packages/Mocking/MRkF3/src/patch.jl:138
│     [11] top-level scope
│        @ REPL[7]:1
│     [12] eval
│        @ ./boot.jl:368 [inlined]
│     [13] eval_user_input(ast::Any, backend::REPL.REPLBackend)
│        @ REPL ~/builds/julia-1.8/usr/share/julia/stdlib/v1.8/REPL/src/REPL.jl:151
│     [14] repl_backend_loop(backend::REPL.REPLBackend)
│        @ REPL ~/builds/julia-1.8/usr/share/julia/stdlib/v1.8/REPL/src/REPL.jl:247
│     [15] start_repl_backend(backend::REPL.REPLBackend, consumer::Any)
│        @ REPL ~/builds/julia-1.8/usr/share/julia/stdlib/v1.8/REPL/src/REPL.jl:232
│     [16] run_repl(repl::REPL.AbstractREPL, consumer::Any; backend_on_current_task::Bool)
│        @ REPL ~/builds/julia-1.8/usr/share/julia/stdlib/v1.8/REPL/src/REPL.jl:369
│     [17] run_repl(repl::REPL.AbstractREPL, consumer::Any)
│        @ REPL ~/builds/julia-1.8/usr/share/julia/stdlib/v1.8/REPL/src/REPL.jl:355
│     [18] (::Base.var"#967#969"{Bool, Bool, Bool})(REPL::Module)
│        @ Base ./client.jl:419
│     [19] #invokelatest#2
│        @ ./essentials.jl:729 [inlined]
│     [20] invokelatest
│        @ ./essentials.jl:726 [inlined]
│     [21] run_main_repl(interactive::Bool, quiet::Bool, banner::Bool, history_file::Bool, color_set::Bool)
│        @ Base ./client.jl:404
│     [22] exec_options(opts::Base.JLOptions)
│        @ Base ./client.jl:318
│     [23] _start()
│        @ Base ./client.jl:522
│
│        nested task error: MethodError: no method matching var"##bar_patch#314"(::Int64)
│        The applicable method may be too new: running in world age 32489, while current world is 32491.
│        Closest candidates are:var"##bar_patch#314"(::Any) at REPL[6]:1 (method too new to be called from this world context.)
│        Stacktrace:
│         [1] macro expansion
│           @ ~/.julia/packages/Mocking/MRkF3/src/mock.jl:24 [inlined]
│         [2] foo(x::Int64)
│           @ Main ./REPL[2]:2
│         [3] (::var"#1#2")(ch::Channel{Any})
│           @ Main ./REPL[5]:2
│         [4] (::Base.var"#591#592"{var"#1#2", Channel{Any}})()
│           @ Base ./channels.jl:134
└ @ Main REPL[7]:3

julia>

If we aren't sure how to fix #89, can we fix this issue in the meantime by changing the call to the alternate to use invokelatest?
i.e. change this:

$alternate_var($args_var...; $(kwargs...))

to this:

    Base.invokelatest($alternate_var, $args_var...; $(kwargs...)) 

?

I don't see any downside there, and it can help avoid catastrophic issues when #89 is encountered.
(I guess the main counter-argument would be that maybe it's better to crash, since this is probably accidental (otherwise the user could just move the patch definition to before the task is spawned) and it's better to have this error be more visible?)

@NHDaly
Copy link
Member Author

NHDaly commented May 23, 2023

The more that I think about this, and talking with our coworkers, I think this is a feature, not a bug.

Imagine that you have tests that set up a server in the background, and then create and test requests to that server with lots of different patches.

It's annoying to have to create a new server for each patch, and it's also annoying and sometimes impossible to have to define all the patches up front, ahead of time.

I'll send up a PR with the above change. it seems like a good improvement to me.

NHDaly added a commit that referenced this issue May 23, 2023
Allow patching a mocked call that is running in an older world age by changing the macro to invoke the patch through `invokelatest`.

The call was already inherently going to be a dynamic dispatch anyway, since by definition the whole point is that we want to substitute in a new function later. Invokelatest will just allow us to substitute in a function that was even _defined_ later, after this function had started running.

This doesn't add any extra overhead, nor introduce any restrictions. The only thing that it does is increase the scenarios that Mocking supports.
NHDaly added a commit that referenced this issue May 31, 2023
[#108] Support mocks running a background task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant