> On Jan. 4, 2016, 8:25 p.m., Maxim Khutornenko wrote: > > src/main/python/apache/thermos/observer/task_observer.py, line 72 > > <https://reviews.apache.org/r/41915/diff/1/?file=1181509#file1181509line72> > > > > We usually put this default initialization directly into the arg list, > > e.g:'stop_event=threading.Event()'. > > John Sirois wrote: > I'm leery of that for mutable objects like an Event. Surprising things > happen if/when the containing object gets constructed a 2nd time and the > single default Event has been mutated! > > John Sirois wrote: > Maxim - your test originally so your call. For a refresh - see here: > https://reviews.apache.org/r/35527/ > > John Sirois wrote: > Disregard comment immediately above - now moved up under Bill's comment > further above where it belongs. > > Maxim Khutornenko wrote: > Not sure how the current approach helps with anything. If a constructor > is called once with `stop_event` and another time without it, wouldn't we end > up in the same state with mutated field due to `stop_event` arg being None > during the second call?
The issue is 2 or more calls without. Say the interleaving goes like this: ``` to1 = TaskObserver(createPathDetector()) to1.stop() to2 = TaskObserver(createPathDetector()) ``` Since `to2` shares `to1`'s Event, its already `set` from the `stop` of `to1` and so never runs. Instead, by using `None` as the sentinel for the default, both `to1` and `to2` each get their own fresh Event. - John ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41915/#review112750 ----------------------------------------------------------- On Jan. 4, 2016, 8:02 p.m., John Sirois wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41915/ > ----------------------------------------------------------- > > (Updated Jan. 4, 2016, 8:02 p.m.) > > > Review request for Aurora, Maxim Khutornenko and Bill Farner. > > > Bugs: AURORA-1570 > https://issues.apache.org/jira/browse/AURORA-1570 > > > Repository: aurora > > > Description > ------- > > Previously, a mock threading.Event was waited on in one thread > and the count of waits was read in another thread. Most thread > memory models do not guaranty reads are fresh in this scenario > unless there is a memory barrier of some sort forcing per-cpu > caches to be flushed. > > This change uses the underlying threading.Event as the memory > barrier instead of mocking it and just wraps the event to record > calls manually. > > src/main/python/apache/thermos/observer/task_observer.py | 5 +++-- > src/test/python/apache/thermos/observer/test_task_observer.py | 36 > ++++++++++++++++++++++++------------ > 2 files changed, 27 insertions(+), 14 deletions(-) > > > Diffs > ----- > > src/main/python/apache/thermos/observer/task_observer.py > 1485de8faef52716f11b82a3556064de26c67427 > src/test/python/apache/thermos/observer/test_task_observer.py > ace15c5305e75fac3a82971f4d71b92bcb37bafc > > Diff: https://reviews.apache.org/r/41915/diff/ > > > Testing > ------- > > Before this change I got a failure between 1/5 and 1/10th of the > time via: > ``` > while true > do > ./pants test src/test/python/apache/thermos/observer/ -- -kTaskObserverTest > done > ``` > > After the change I cannot trigger the failure. > > > Thanks, > > John Sirois > >
