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

Don't share state in build task #3946

Merged
merged 2 commits into from Apr 19, 2018
Merged

Don't share state in build task #3946

merged 2 commits into from Apr 19, 2018

Conversation

agjohnson
Copy link
Contributor

Move task steps to an isolated class, instead of possibly sharing state in
between tasks. This bug is not consistently reproduceable, but one consistent
way to reproduce is to drop to rdb on the UpdateDocsTask run method.
Previously, if you did this while running 2 celery processes, and triggered 2
builds, you could witness state changing on the task instance. By moving state
to a separate class that is instantiated on each task execution, this completely
avoids shared state in these tasks.

Move task steps to an isolated class, instead of possibly sharing state in
between tasks. This bug is not consistently reproduceable, but one consistent
way to reproduce is to drop to ``rdb`` on the UpdateDocsTask run method.
Previously, if you did this while running 2 celery processes, and triggered 2
builds, you could witness state changing on the task instance. By moving state
to a separate class that is instantiated on each task execution, this completely
avoids shared state in these tasks.
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This was a good deal less intrusive than I was thinking in the abstract. I believe this will introduce a number of downstream bugs where we were accessing state that was stale, but throwing exceptions instead of using bad data is good. 👍

@ericholscher
Copy link
Member

I do think we should have docstrings here explaining the design though. Otherwise I could see this getting refactored away as unneeded complexity.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

I like the approach used here, 👍

I'd like to echo what Eric said regarding writing a good docstring since it wasn't clear to find the issue and also we don't have a good way to write a test for this.

@humitos
Copy link
Member

humitos commented Apr 14, 2018

I want to add some context here that I found while reading how to use the celery framework instead a custom workaround to fix this:

Those two links, talks about similar issues.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

👍

@agjohnson
Copy link
Contributor Author

Opened #3973 and #3974 to address some of the issues not addressed here.

@agjohnson agjohnson merged commit df25dbc into master Apr 19, 2018
@agjohnson agjohnson deleted the agj/isolate-build-task branch April 19, 2018 16:39
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.

None yet

3 participants