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

gossip.unregister_token does not work as expected #25

Closed
ayalash opened this issue Feb 26, 2017 · 4 comments
Closed

gossip.unregister_token does not work as expected #25

ayalash opened this issue Feb 26, 2017 · 4 comments
Assignees

Comments

@ayalash
Copy link
Collaborator

ayalash commented Feb 26, 2017

The following code fails on AssertionError:

import gossip

@gossip.register('a.b', token='token')
def foo():
    pass

gossip.unregister_token('token')
assert not gossip.get_all_registrations()  # <= Raise AssertionError
@ayalash ayalash self-assigned this Feb 26, 2017
@ayalash
Copy link
Collaborator Author

ayalash commented Feb 26, 2017

gossip.untergister_token calls gossip.get_global_group().unregister_token(token).
There are two bugs in the code of Gossip.unregister_token, that I found so far:

  • It uses Group.iter_hooks which does not iterate the hooks of its subgroups.
  • It invalidate all registrations of each of its hooks by using Hook.get_registrations. The method does not returns the empty registration of the hook.

@ayalash
Copy link
Collaborator Author

ayalash commented Feb 26, 2017

@vmalloc I just saw this method (during my debugging of this issue):

In [10]: gossip.registration.Registration.can_be_called??
Signature: gossip.registration.Registration.can_be_called(self)
Source:
    def can_be_called(self):
        return True
File:      ~/src/github/gossip/gossip/registration.py
Type:      function

Do you remember what is the purpose of this method?
I didn't find any code that uses it (not even unittests).

Should we change the return value of this method (to False) for empty registrations?

@vmalloc
Copy link
Member

vmalloc commented Feb 28, 2017

@ayalash I think it's a remainder of an old code for computing dependencies

@vmalloc
Copy link
Member

vmalloc commented Feb 28, 2017

Doesn't seem to be needed anymore though

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

2 participants