JuliaLogging / TerminalLoggers.jl

Logging sinks and utilites for interactive terminals
MIT License
41 stars 10 forks source link

task switch not allowed from inside gc finalizer #6

Closed tkf closed 4 years ago

tkf commented 4 years ago

Maybe we need to do something like https://github.com/JuliaLang/julia/pull/33698?

julia> with_logger(TerminalLogger()) do
           @info "hello" sticky=true
       end

julia> @async GC.gc()

error in running finalizer: ErrorException("task switch not allowed from inside gc finalizer")
Task (done) @0x00007f3689b89600
c42f commented 4 years ago

Oh yeah that's nasty :grimacing:

I think we need to improve with_logger so that it sends some kind of "attach" and "detach" events to the logger. (Cf Transducers.start / Transducers.complete)

c42f commented 4 years ago

In the meantime would it work to use Core.write to synchronously reset the scrolling region from the finalizer?

tkf commented 4 years ago

I think we need to improve with_logger so that it sends some kind of "attach" and "detach" events to the logger.

Yeah, I think that's a good idea. I was thinking a similar API for with_logger and global_logger (on_uninstall or something).

use Core.write

This sounds like a nice hack (at least for now).

c42f commented 4 years ago

Oh, I think I misunderstood your suggestion. We could certainly just use @async from the finalizer (ensuring it calls showsticky directly to avoid prolonging the StickyMessages lifetime?)

tkf commented 4 years ago

Both Core.write and @async sound OK to me. I guess I prefer @async as it's a public API.

c42f commented 4 years ago

Ok, how about #7? I wasn't sure it made sense to use empty! because that prolongs the life of the sticky messages object. Though I guess I'm prolonging the life of the IO which is held by the StickyMessages which is just as weird in principle. Either way, finalizers are definitely a poor tool for this. (I think that's much clearer to me now than when I originally wrote this code...)

tkf commented 4 years ago

7 looks good to me. It's nice that showsticky does not need StickyMessages to do this.