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

fix: Log exception and exit #505

Merged
merged 2 commits into from
Mar 2, 2017
Merged

Conversation

giulioungaretti
Copy link
Contributor

Closes #504

Changes proposed in this pull request:

  • log exception in background task
  • exit loop

@alan-geller , and @QCoDeS/core the question here is if we want to exit (early return) or not.
My feeling is that we may as well not exit, technically the data could be still good even if the background task failed, but one never knows.

The clean way is to return, but I am curious about your 💎 feedback !

@giulioungaretti giulioungaretti modified the milestone: v0.1.3 Mar 1, 2017
Copy link

@alan-geller alan-geller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is of course fine.

The original use for the background task was to update a live plot. In this case, I don't think the task throwing an exception should cause the loop to exit.

Other background tasks, of course, might properly cause the loop to fail if the task fails (throws).

I think it's reasonable to always fail if the background task throws, since you can always wrap the "real" background task in a try/catch that just ignores all exceptions (presumably other than keyboard interrupt). We should make sure that the docs call out this behavior.

As an aside, I would vote (strongly!) against adding another optional argument that flagged whether or not the loop should exit on a background task exception... There are far too many options already.

@alexcjohnson
Copy link
Contributor

The logging is a great idea. I agree with @alan-geller though that we shouldn't exit on background task failure, unless the things people are doing with background tasks have changed substantially - data collection should not be contingent on live plotting, especially since the user can manually quit the loop if they're not seeing what they want.

Originally the code had the last_task_failed flag - the idea here was to allow the background task to fail once without being removed, following the idea "once is never, twice is always." I seem to recall seeing this with live plotting when the original draw may have had no data in it, but the second draw, when there is some data, would work? Of course that bug could be fixed, but who knows what else will come up particularly with Qt.

@giulioungaretti
Copy link
Contributor Author

@alexcjohnson @alan-geller I am not sure I like the "once is never, twice is always." But let's not change it for now.
So no early return, log the exception and remove the task if fail twice.

giulio ungaretti and others added 2 commits March 2, 2017 13:29
According to this principle from Alej "once is never, twice is always."
See pr comments.
@giulioungaretti giulioungaretti merged commit 20bbdd9 into master Mar 2, 2017
@giulioungaretti giulioungaretti deleted the fix/logBackgroundException branch March 2, 2017 12:35
giulioungaretti pushed a commit that referenced this pull request Mar 2, 2017
Author: Giulio Ungaretti <[email protected]>

    fix: Log exception and exit (#505)
giulioungaretti pushed a commit that referenced this pull request Mar 2, 2017
Author: Giulio Ungaretti <[email protected]>

    fix: Log exception and exit (#505)
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 this pull request may close these issues.

3 participants