Project

Profile

Help

Task #2318

closed

Ensure thread-safety of ProgressReport models

Added by bmbouter over 7 years ago. Updated over 4 years ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
Normal
Category:
-
Sprint/Milestone:
Start date:
Due date:
% Done:

100%

Estimated time:
Platform Release:
Groomed:
Yes
Sprint Candidate:
Yes
Tags:
Documentation
Sprint:
Sprint 11
Quarter:

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

Actions #1

Updated by mhrivnak over 7 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.

Actions #2

Updated by bmbouter over 7 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.

Actions #3

Updated by mhrivnak over 7 years ago

  • Groomed changed from No to Yes
Actions #4

Updated by mhrivnak over 7 years ago

  • Sprint/Milestone set to 29
Actions #5

Updated by dkliban@redhat.com over 7 years ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to dkliban@redhat.com
Actions #6

Updated by dkliban@redhat.com over 7 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.

Actions #7

Updated by bmbouter over 7 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.

Actions #8

Updated by dkliban@redhat.com over 7 years ago

  • Status changed from ASSIGNED to POST

Added by dkliban@redhat.com over 7 years ago

Revision ee3ec288 | View on GitHub

Adds docs about using ProgressBar with threads

closes #2318 https://pulp.plan.io/issues/2318

Added by dkliban@redhat.com over 7 years ago

Revision ee3ec288 | View on GitHub

Adds docs about using ProgressBar with threads

closes #2318 https://pulp.plan.io/issues/2318

Actions #10

Updated by dkliban@redhat.com over 7 years ago

  • Status changed from POST to MODIFIED
  • % Done changed from 0 to 100
Actions #11

Updated by bmbouter about 6 years ago

  • Sprint set to Sprint 11
Actions #12

Updated by bmbouter about 6 years ago

  • Sprint/Milestone deleted (29)
Actions #13

Updated by daviddavis almost 5 years ago

  • Sprint/Milestone set to 3.0.0
Actions #14

Updated by bmbouter almost 5 years ago

  • Tags deleted (Pulp 3)
Actions #15

Updated by bmbouter over 4 years ago

  • Status changed from MODIFIED to CLOSED - CURRENTRELEASE

Also available in: Atom PDF