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.

Sign up for GitHub

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

Jump to bottom

Callers of Future.callback= can cause hangs #6849

Open
RedBeard0531 opened this issue Dec 1, 2017 · 0 comments
Open

Callers of Future.callback= can cause hangs #6849

RedBeard0531 opened this issue Dec 1, 2017 · 0 comments
Labels
Async Everything related to Nim's async Standard Library

Comments

@RedBeard0531
Copy link
Contributor

Unlike Future.addCallback, Future.callback= clears the callback list which prevents previously registered listeners from being notified. This can cause hard to debug hangs. I ran into this when using all() to wait for all input edges in a graph, some of which were also transitive dependencies. I think Future.callback= should be deprecated and possibly removed in favor of addCallback. If there are cases where the existing behavior is needed, clearCallbacks should be made public so that it can be done explicitly.

I'm about to file a PR to fix all(). I tried doing a wider sweep of callback= but that caused some tests to fail and I didn't have time or experience to really dig into them.
RedBeard0531 added a commit to RedBeard0531/Nim that referenced this issue Dec 1, 2017
@RedBeard0531
Use addCallback rather than callback= in asyncfutures.all()
66e309a 
Addresses part of nim-lang#6849
@RedBeard0531 RedBeard0531 mentioned this issue Dec 1, 2017
Merged
RedBeard0531 added a commit to RedBeard0531/Nim that referenced this issue Dec 8, 2017
@RedBeard0531
Use addCallback rather than callback= in asyncfutures.all()
d4409a6 
Addresses part of nim-lang#6849
RedBeard0531 added a commit to RedBeard0531/Nim that referenced this issue Dec 9, 2017
@RedBeard0531
Stop using do notation for nim-lang#6849
a5cd360
dom96 pushed a commit that referenced this issue Dec 9, 2017
@RedBeard0531 @dom96
Use addCallback rather than callback= in asyncfutures.all() (#6850)
b87ef65 
* Use addCallback rather than callback= in asyncfutures.all()

Addresses part of #6849

* Stop using do notation for #6849

* Update example style
@Yardanico Yardanico added Async Everything related to Nim's async Standard Library labels Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Async Everything related to Nim's async Standard Library
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@RedBeard0531 @Yardanico