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

Maintenance for AsyncBulkCall #238

Merged

Conversation

maxfischer2781
Copy link
Member

This PR fixes some internal issues of AsyncBulkCall. Major changes include:

  • correctly call task_done for each item read from the queue
  • maintain strong references for bulk tasks to avoid aborting them (see bpo#44665)
  • clean up the dispatch task when there is no work to do

Closes #232.


Note: I considered cleaning up the dispatch task via a callback, the way bulk tasks are cleaned up. However, I could not find guarantees on promptness so had to assume using a callback may introduce a race between shutting down _bulk_dispatch and __call__ detecting that it needs to restart dispatch.

@maxfischer2781 maxfischer2781 requested review from a team, giffels and mschnepf and removed request for a team April 7, 2022 10:16
@maxfischer2781
Copy link
Member Author

@giffels The newest flake8-bugbear release adds B019, which does not like our use of LRU caches on methods. Should I disable the check?

mschnepf
mschnepf previously approved these changes Apr 7, 2022
Copy link
Member

@mschnepf mschnepf left a comment

Choose a reason for hiding this comment

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

I have just one comment/question, But nothing critical. Thanks for your work 👍

tests/utilities_t/test_asyncbulkcall.py Outdated Show resolved Hide resolved
Copy link
Member

@mschnepf mschnepf left a comment

Choose a reason for hiding this comment

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

Thanks for the change. Now it is less confusing. Merge it so 🖖

mschnepf
mschnepf previously approved these changes Apr 7, 2022
Copy link
Member

@mschnepf mschnepf left a comment

Choose a reason for hiding this comment

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

Looks fine. Thanks for your work.

@giffels
Copy link
Member

giffels commented Apr 12, 2022

@giffels The newest flake8-bugbear release adds B019, which does not like our use of LRU caches on methods. Should I disable the check?

Yes, please disable it.

Copy link
Member

@giffels giffels left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work. I will merge it once B019 is disabled.

Copy link
Member

@giffels giffels left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work! This PR is ready to meeeeeerrrrrrggggeeee! ;-)

Copy link
Member

@mschnepf mschnepf left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@giffels giffels merged commit 8f1cd47 into MatterMiners:master Apr 13, 2022
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.

Missing task done call in AsyncBulkCall
3 participants