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

Instantiate error for each future.set_exception #247

Merged
merged 2 commits into from
Nov 17, 2017

Conversation

Artimi
Copy link

@Artimi Artimi commented Oct 27, 2017

This fixes #246 . I've found that set_exceptions is used also in MessageBatch.failure() but it is settings an exception that is passed as argument. I would need to change it's interface to pass some function that returns the exception so it can be called for every future in MessageBatch._msg_future. Do you agree or do you have another idea?

@tvoinarovskyi
Copy link
Member

Sorry for the long wait on this, still checking if there's a more general way of fixing it.

@Artimi
Copy link
Author

Artimi commented Nov 10, 2017

@tvoinarovskyi Have you found something? I was trying to find this bug directly in Python but with no success. This issue is a time bomb for us because it can effectively kill one of our service. What if we use copy.deepcopy in MessageBatch?

@Artimi
Copy link
Author

Artimi commented Nov 13, 2017

@tvoinarovskyi I added the version with copy. Now it could be ready to merge if you agrees.

future.set_exception(exception)
# we need to copy exception so traceback is not multiplied
# https://github.com/aio-libs/aiokafka/issues/246
future.set_exception(copy.deepcopy(exception))
Copy link
Member

Choose a reason for hiding this comment

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

Seems like copy.copy() is enough to prevent traceback changes. The tracebacks are immutable and linked in a linked list, so copy is enough.
For example:

import copy


def test_raise_many():
    err = ValueError()
    for i in range(100):
        try:
            raise copy.copy(err)
        except Exception:
            pass
    raise err

test_raise_many()

Copy link
Member

Choose a reason for hiding this comment

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

@tvoinarovskyi
Copy link
Member

Sorry, was away for some time. Please use copy and lets merge it.

@Artimi Artimi force-pushed the multiplied-exception-traceback branch from 9160162 to a97d6a5 Compare November 16, 2017 14:25
@Artimi
Copy link
Author

Artimi commented Nov 16, 2017

@tvoinarovskyi Done. Thanks for investigation.

@tvoinarovskyi tvoinarovskyi merged commit 1e07a87 into aio-libs:master Nov 17, 2017
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.

Exception traceback is multiplied by number of futures
2 participants