Task #2318
closedEnsure thread-safety of ProgressReport models
100%
Description
Django models changes can be thread safe[0], but some care needs to be taken. We need the Progressreport models specifically to be thread safe.
This likely involves 2 things:
1. rewriting the implementaiton to be thread safe
2. Using documentation which identifies the thread-safety concern for plugin writers updating models themselves from many threads.
[0]: http://stackoverflow.com/questions/26234819/are-django-models-thread-safe#26234946
Updated by mhrivnak about 8 years ago
It is likely just the incrementing feature that needs to be thread-safe. The pattern will probably be:
- create a progress object
- create a Queue or similar
- create some threads and give them a reference to the queue, and probably the incrementor that comes with the progress object
- each thread runs in a loop: grab a job from the queue, do it, call queue.task_done(), and increment the progress object
- the main thread calls queue.join() and waits for the work to finish
It's worth considering if there's a way to either wrap or somehow hook into the queue's task_done method, but that's optional. There may not be a reasonable way to do so.
Updated by bmbouter about 8 years ago
Yes item 1 is likely as simple as the increment() method only being updated. I think most of the work is in the docs.
Updated by dkliban@redhat.com almost 8 years ago
- Status changed from NEW to ASSIGNED
- Assignee set to dkliban@redhat.com
Updated by dkliban@redhat.com almost 8 years ago
https://github.com/pulp/pulp/pull/2871#issuecomment-264014822
Based on the conversation on the above PR, it sounds like we only need to add some documentation that states that a single instance of a ProgressBar or ProgressSpinner objects should be used by all threads when processing items in parallel.
Updated by bmbouter almost 8 years ago
- Description updated (diff)
- Tags Documentation added
After some lunchtime conversation, it was confirmed that this is a documentation only change and that the docstrings should recommend the user have all threads share the same instance of ProgressBar. I don't think ProgressSpinner and ProgressReport need updates since updating those frequently will almost never occur.
I'm setting the documentation tag and rewriting the story some.
Updated by dkliban@redhat.com almost 8 years ago
- Status changed from ASSIGNED to POST
Added by dkliban@redhat.com almost 8 years ago
Added by dkliban@redhat.com almost 8 years ago
Revision ee3ec288 | View on GitHub
Adds docs about using ProgressBar with threads
Updated by dkliban@redhat.com almost 8 years ago
- Status changed from POST to MODIFIED
- % Done changed from 0 to 100
Applied in changeset pulp|ee3ec2888d5ceb8c9faa6819e61047a61b3841df.
Updated by bmbouter almost 5 years ago
- Status changed from MODIFIED to CLOSED - CURRENTRELEASE
Adds docs about using ProgressBar with threads
closes #2318 https://pulp.plan.io/issues/2318