Allow the last callback called to add a callback #1373
Conversation
This fixes a bug in the finished and response callbacks where if the last/only callback adds another callback, the newly added callback won't be called afterwards. This is because when it tries to add the callback, it is added to a new list instance because the callbacks list is empty at that time; the check for whether the callbacks list was created didn't previously distinguish between an empty list and not a list. However, if it is not the last callback in the list, the callbacks list will not be empty and the new callback will be added to the same list and the newly added callback *will* be called. Because the code as written appears to be trying to support callbacks adding callbacks, this push request modifies the code so that a callback may add another callback whether it is the last one or not. An alternative approach would be to modify the code so that callbacks cannot add new callbacks, which also would be reasonable. But I think it's a bug that the behavior depends currently on whether you are in the last/only callback when you try to add another one.
|
I'm not sure I like your chosen fix, however the issue should be corrected. Perhaps with an |
…en it is the only callback.
|
I added a test case that will reproduce the problem for response_callbacks. I'm not sure what you are mean when you say "Perhaps an |
|
|
|
Hi, I changed it to use None. I also did a version using a deque instead of a list, it seems suitable for the task. But it's a separate commit so it can easily be rolled back if you don't want it. |
Allow the last callback called to add a callback
|
This looks good to me, thanks for the fixes! |
This fixes a bug in the finished and response callbacks where if the last/only callback adds another callback, the newly added callback won't be called afterwards.
This is because when it tries to add the callback, it is added to a new list instance because the callbacks list is empty at that time; the check for whether the callbacks list was created didn't previously distinguish between an empty list and not a list. However, if it is not the last callback in the list, the callbacks list will not be empty and the new callback will be added to the same list and the newly added callback will be called.
Because the code as written appears to be trying to support callbacks adding callbacks, this push request modifies the code so that a callback may add another callback whether it is the last one or not.
An alternative approach would be to modify the code so that callbacks cannot add new callbacks, which also would be reasonable. But I think it's a bug that the behavior depends currently on whether you are in the last/only callback when you try to add another one.