Repository navigation
Add ScopedJobThreadPtr class to be used with JobThread - #4275
Conversation
67a8d3b to
30e7e60
Compare
|
@borongc is it possible for you to help me review the part in this PR where all changes to Schedule() made during the scoped_ptr<> to unique_ptr<> migration is reverted? |
30e7e60 to
5a37123
Compare
|
@osagie98 @sideb0ard is it possible for you to take a look at this one? |
5a37123 to
1808d62
Compare
looking now, re-reading bug history. |
sideb0ard
left a comment
There was a problem hiding this comment.
From reading Xiaoming and Jelle's comments, specifically https://b.corp.google.com/issues/372515171#comment11
this new ScopedJobThreadPtr has the behavior we require in the correct order, i.e. finish jobs, terminate thread, set pointer to null.
However, as this PR and bug are still open, we appear to still have some lingering reservations.
Suggestion:
- Can we first add a new test to job_thread_test.cc 'ScheduleJobDuringShutdown', which specifically triggers the DCHECK listed in https://b.corp.google.com/issues/372515171#comment1
This new test should FAIL with the current implementation. - Then, secondly, run the new test with this PR, which should PASS. If it does pass, then we can be sure it's solved the issue as we currently understand it.
I've created PR for ScheduleJobDuringShutdown test: #4864 |
1808d62 to
3fa0391
Compare
|
I've added ScheduleJobDuringShutdown test to this PR. |
|
With this change, I think And we should also have another change to update tvOS code. |
This PR will be followed by CL for the internal part. |
I see, we can remove it in a follow up cl then. |
|
|

b/372515171