Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2262043002:
MidiScheduler binds to task runner of constructing thread (Closed)
|
Created:
4 years, 4 months ago by Shao-Chuan Lee Modified:
4 years, 4 months ago Reviewers:
Takashi Toyoshima CC:
chromium-reviews, feature-media-reviews_chromium.org, toyoshim+midi_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMidiScheduler binds to task runner of constructing thread
Now PostSendDataTask() posts to the bound task runner, and ensures that constructor/destructor and InvokeClosure() are called on the same thread.
R=toyoshim@chromium.org
Committed: https://crrev.com/37e75a069c8d9ec4de2b6af29055633b76bbaf3b
Cr-Commit-Position: refs/heads/master@{#413981}
Patch Set 1 #Patch Set 2 : bind to constructing thread #
Total comments: 7
Patch Set 3 : fix MidiScheduler misuse #Patch Set 4 : lock for safe MidiScheduler destruction #Patch Set 5 : comments #Patch Set 6 : rebase #
Messages
Total messages: 40 (23 generated)
Thank you for making this. While I'm reviewing this change, I find a race issue in this approach. See base/memory/weak_ptr.h. WeakPtr expects instances are dereferenced and invalidated on the same same. But in this approach, it is invalidated on the thread that MidiScheduler is destructed, and dereferenced on the passed task runner's thread. This causes a race. To avoid this problem, there may be some approaches. Plan 1) Change to pass the task_runner at the constructor so that we can ensure one specified thread always dereferences the weak pointer. Then, invalidate it on the specified task_runner. Probably, this may need another mutex or event to make it work correctly. Plan 2) MidiScheduler runs all posted tasks on the thread on which MidiScheduler is constructed. Then construct the scheduler on the com thread in your case. To realize this, the MidiScheduler constructor needs to take a reference to the current task_runner so to use it on posting a delayed task, and makes PostSendDataTask method thread-safe.
Optional: You may write a simple unit test to check thread safety by running itwith tsan enabled.
Now MidiScheduler binds to the thread on which it's constructed, to ensure constructor/destructor and InvokeClosure() run on the same thread. https://codereview.chromium.org/2262043002/diff/20001/media/midi/midi_schedul... File media/midi/midi_scheduler.cc (right): https://codereview.chromium.org/2262043002/diff/20001/media/midi/midi_schedul... media/midi/midi_scheduler.cc:37: weak_factory_.GetWeakPtr(), Assuming GetWeakPtr() is thread-safe.
code looks good, but I have some requests to add more comments to avoid someone mistakenly break something in the future. https://codereview.chromium.org/2262043002/diff/20001/media/midi/midi_schedul... File media/midi/midi_scheduler.cc (right): https://codereview.chromium.org/2262043002/diff/20001/media/midi/midi_schedul... media/midi/midi_scheduler.cc:37: weak_factory_.GetWeakPtr(), OK, but probably, it's better to have a unit test to use this class from multiple-thread to ensure this keeps working correctly in the future. TODO is fine. https://codereview.chromium.org/2262043002/diff/20001/media/midi/midi_schedul... File media/midi/midi_scheduler.h (right): https://codereview.chromium.org/2262043002/diff/20001/media/midi/midi_schedul... media/midi/midi_scheduler.h:29: // Post |closure| to |task_runner| safely. The |closure| will not be invoked Now that this method does not take |task_runner|, you need to rephrase it. Also, probably we should explicitly say that only this method can be called from any thread. https://codereview.chromium.org/2262043002/diff/20001/media/midi/midi_schedul... media/midi/midi_scheduler.h:45: // The TaskRunner of the thread on which the instance is constructed. Can we add some comments why it's fine that we can read task_runner_ without lock inside PostSendDataTask? I think it contains following points. - task_runner_ is modified only inside ctor and dtor - the pointer to the instance should be ensured by an external lock to be used in multiple-threads. - When the pointer to the instance is valid to the caller thread inside the lock, task_runner_ is also valid.
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Also, I kicked CQ dry run because the code already looks good as far as I can check in the code review.
The CQ bit was checked by shaochuan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add task_runner argument to MidiScheduler::PostSendDataTask Now PostSendDataTask accepts an optional |task_runner| argument for posting the closure to a specific task runner. |task_runner| defaults to the task runner of current message loop. R=toyoshim@chromium.org ========== to ========== MidiScheduler binds to task runner of constructing thread Now PostSendDataTask() posts to the bound task runner, and ensures that constructor/destructor and InvokeClosure() are called on the same thread. R=toyoshim@chromium.org ==========
Looks good except for |task_runner| I commented at #5. Once CL description is updated and bots are happy, I'm find to say l-g-t-m.
https://codereview.chromium.org/2262043002/diff/20001/media/midi/midi_schedul... File media/midi/midi_scheduler.cc (right): https://codereview.chromium.org/2262043002/diff/20001/media/midi/midi_schedul... media/midi/midi_scheduler.cc:37: weak_factory_.GetWeakPtr(), On 2016/08/23 05:30:41, toyoshim wrote: > OK, but probably, it's better to have a unit test to use this class from > multiple-thread to ensure this keeps working correctly in the future. TODO is > fine. Added TODO. https://codereview.chromium.org/2262043002/diff/20001/media/midi/midi_schedul... File media/midi/midi_scheduler.h (right): https://codereview.chromium.org/2262043002/diff/20001/media/midi/midi_schedul... media/midi/midi_scheduler.h:29: // Post |closure| to |task_runner| safely. The |closure| will not be invoked On 2016/08/23 05:30:41, toyoshim wrote: > Now that this method does not take |task_runner|, you need to rephrase it. > Also, probably we should explicitly say that only this method can be called from > any thread. Should be |task_runner_|. Added comments. https://codereview.chromium.org/2262043002/diff/20001/media/midi/midi_schedul... media/midi/midi_scheduler.h:45: // The TaskRunner of the thread on which the instance is constructed. On 2016/08/23 05:30:41, toyoshim wrote: > Can we add some comments why it's fine that we can read task_runner_ without > lock inside PostSendDataTask? > > I think it contains following points. > - task_runner_ is modified only inside ctor and dtor > - the pointer to the instance should be ensured by an external lock to be used > in multiple-threads. > - When the pointer to the instance is valid to the caller thread inside the > lock, task_runner_ is also valid. Added comments at PostSendDataTask().
lgtm
The CQ bit was checked by toyoshim@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by shaochuan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by shaochuan@chromium.org
The CQ bit was checked by shaochuan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by shaochuan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from toyoshim@chromium.org Link to the patchset: https://codereview.chromium.org/2262043002/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MidiScheduler binds to task runner of constructing thread Now PostSendDataTask() posts to the bound task runner, and ensures that constructor/destructor and InvokeClosure() are called on the same thread. R=toyoshim@chromium.org ========== to ========== MidiScheduler binds to task runner of constructing thread Now PostSendDataTask() posts to the bound task runner, and ensures that constructor/destructor and InvokeClosure() are called on the same thread. R=toyoshim@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== MidiScheduler binds to task runner of constructing thread Now PostSendDataTask() posts to the bound task runner, and ensures that constructor/destructor and InvokeClosure() are called on the same thread. R=toyoshim@chromium.org ========== to ========== MidiScheduler binds to task runner of constructing thread Now PostSendDataTask() posts to the bound task runner, and ensures that constructor/destructor and InvokeClosure() are called on the same thread. R=toyoshim@chromium.org Committed: https://crrev.com/37e75a069c8d9ec4de2b6af29055633b76bbaf3b Cr-Commit-Position: refs/heads/master@{#413981} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/37e75a069c8d9ec4de2b6af29055633b76bbaf3b Cr-Commit-Position: refs/heads/master@{#413981} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
