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

Log errors for unhandled task exceptions #34279

Closed
twavv opened this issue Jan 6, 2020 · 8 comments
Closed

Log errors for unhandled task exceptions #34279

twavv opened this issue Jan 6, 2020 · 8 comments

Comments

@twavv
Copy link
Contributor

twavv commented Jan 6, 2020

This has been discussed a few times, most notably in #10405, but without real resolution (just some gesturing that it would be a good idea to fix things).

It would be _really nice_™ for unhandled exceptions in tasks to print some kind of error. I can think of two options.

  • In JavaScript, a rejected promise is considered unhandled if there are no listeners at the time of the rejection. This works because promises are not resolved until the next tick of the event loop (so there's no race condition between constructing a promise and attaching the handler, as long as there are no awaits in between).
  • In Python, futures are considered unhandled if they are resolved with an error and then garbage collected without checking on that error.

The latter seems like the better option (in my humble opinion).

Would it be possible to attach a finalizer to the Task struct by default (as well as some kind of error handled flag) that would print an error message when the task is finalized and the error has never been retrieved?

@twavv
Copy link
Contributor Author

twavv commented Jan 6, 2020

Related: #12403

Not sure why that didn't show up in my search earlier.

@c42f
Copy link
Member

c42f commented Jan 7, 2020

See also #12736 for discussion of a previous attempt to solve this. I believe a finalizer would solve the problems which caused that version of the warning to be reverted. Adding more finalizers is always a bit disappointing though. It would be more palatable if there was a way to opt out. For example, arrange so that it's only done when the task is created outside a @sync block.

(See also also some analogous discussion at #33698, including mention of some pitfalls about emitting warnings from finalizers.)

@twavv
Copy link
Contributor Author

twavv commented Jan 7, 2020

For example, arrange so that it's only done when the task is created outside a @sync block.

My understanding is that a @sync block always calls wait for all of the tasks that are created inside of it. A brief look at the @sync and Base.sync_end suggests that even if some tasks error, all tasks are ultimately wait-ed, so it shouldn't be an issue. We could also add an argument to the Task constructor to disable the warning message.

It would be a nice thing™ to have some way of silencing those warnings altogether (environment variable?) but we might not even want that, to be honest (for example, Python has no way of silencing - as far as I can tell - warnings about unretrieved exceptions for Futures).

@c42f
Copy link
Member

c42f commented Jan 8, 2020

We could also add an argument to the Task constructor to disable the warning message.

Yes, I think that would be roughly the way we'd have to do it in the current system; it looks like the GC's finalizer system isn't compatible with the idea of "removing a finalizer" so we'd have to arrange that it's not added in the first place.

Personally I think we should jump on the structured concurrency bandwagon (#33248) which I feel would be a general solution to these kind of problems (it should ensure the parent task is always available to receive any errors from failing children — roughly, imagine that @sync was always required and work through the consequences of that.)

@twavv
Copy link
Contributor Author

twavv commented Jan 9, 2020

Yes, I think that would be roughly the way we'd have to do it in the current system; it looks like the GC's finalizer system isn't compatible with the idea of "removing a finalizer" so we'd have to arrange that it's not added in the first place.

I think this could just be handled by adding a flag to the Task struct.

@c42f
Copy link
Member

c42f commented Jan 9, 2020

I think this could just be handled by adding a flag to the Task struct.

There's already special GC code for handling Tasks so that could possibly work. Calling back into julia as a result of some GC work is tricky though. Perhaps if the warning was deferred by adding to the finalizer list (or some such thing) it could work out.

@jonathan-laurent
Copy link

jonathan-laurent commented Apr 26, 2021

I remember this issue cost me a lot of time and frustration. Although I do not perfectly understand the technical debate around this issue, I believe that avoiding silent failures is important and worth some technical compromises.

Right now, I am wrapping every task that is spawned in my code with the following macro:

"""
    @printing_errors expr

Evaluate expression `expr` while printing any uncaught exception on `stderr`.
"""
macro printing_errors(expr)
  return quote
    try
      $(esc(expr))
    catch e
      showerror(stderr, e, catch_backtrace())
    end
  end
end

Is anyone aware of a better workaround?

@vtjnash
Copy link
Member

vtjnash commented Apr 26, 2021

Closed by #39518

@vtjnash vtjnash closed this as completed Apr 26, 2021
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

No branches or pull requests

4 participants