WebAgg backend: Fix unbound variable error in get_diff_image #8656
Conversation
… called while self._png_is_old
|
|
||
| return buff | ||
| self.buff = buff | ||
| return self.buff |
There was a problem hiding this comment.
Would it be better to return buff inside the if block and return None if there is no changes?
There was a problem hiding this comment.
semantically yes, but the consumer of that return value in refresh_all then complains that None is not a buffer. (send_binary, that is)
def refresh_all(self):
if self.web_sockets:
diff = self.canvas.get_diff_image()
for s in self.web_sockets:
s.send_binary(diff)
There was a problem hiding this comment.
I guess refresh_all could inspect self.canvas._png_is_old to see whether there is a need to refresh, and otherwise just do nothing (no sending of binaries). but that's not entirely clean in terms of attribute access
There was a problem hiding this comment.
Maybe in refresh_all it should only send the diff if it is non None? What does the consumer side do if it receives the same diff multiple times?
👎 on looking at private attributes.
There was a problem hiding this comment.
That sounds like the sanest way to do it. I've updated the PR
|
Thx for the quick reaction everyone! |
|
Congratulations on your first contribution to Matplotlib 🎉 ! Hopefully we will hear from you again. It is unlikely that there will be a 2.0.3, but 2.1 is targeted for July. |
|
Cheers for running a welcoming project! July-ish sounds perfectly reasonable for us. Thanks again |
PR Summary
FigureCanvasWebAggCore.get_diff_image()can crash with an UnboundLocalError on its return valuebuff. This is becausebuffis only created whenself._png_is_old == True, but get_diff_image tries to return it in either case.We presumably ran into this because tornado likes to call
FigureManagerWebAgg.refresh_allmultiple times in between calls to draw() in certain situations -- then, even if draw() did set the _png_is_old flag, refresh_all resets it through its call to get_diff_image, and so the next refresh will fail.This change keeps the buffer around as an attribute of the Canvas, so get_diff_image can return it regardless of whether it has been refreshed.This change lets get_diff_image return None when the png isn't outdated and adds a condition to
refresh_allto only send diffs that are non-None.