PSA: Removing some uses of MessageLoop::current()

[Francois Pierre Doray]

FYI, the current replacements are about to be applied to the codebase:

Before	After
MessageLoop::current()->    PostTask    PostDelayedTask    DeleteSoon    ReleaseSoon	ThreadTaskRunnerHandle::Get()->    PostTask    PostDelayedTask    DeleteSoon    ReleaseSoon
MessageLoop::current()->task_runner()	ThreadTaskRunnerHandle::Get()
MessageLoop::current()->Run()	RunLoop.Run()

These replacements don’t change code behavior.

Why is this happening?

• The fact that there's a MessageLoop on the thread is an unnecessary implementation detail. When browser threads are migrated to base/task_scheduler [1], tasks will no longer have access to a MessageLoop.

• MessageLoop::PostTask/PostDelayedTask/DeleteSoon/ReleaseSoon/Run are deprecated.

Follow progress: https://crbug.com/616447

[1] Design doc visible to @chromium.org and those who request access.  

[Ryan Sleevi]

From the bug, it appears you're also removing instances of MessageLoopForUI or MessageLoopForIO as well ( e.g. https://bugs.chromium.org/p/chromium/issues/detail?id=616447#c6 and https://bugs.chromium.org/p/chromium/issues/detail?id=616447#c7 )

Using base::ThreadTaskRunnerHandle is a weaker API contract than either of those two cases, and can make it unclear what the assumptions, guarantees, or conditions of execution are.

Your design doc doesn't mention the plan to transition these (AFAICT, but I was simply basic on search for "MessageLoopFor")

What is your recommended pattern for these? The old pattern included additional DCHECKs to ensure correctness about the threading assumptions, but your proposed transition plan effectively removes these DCHECKs, making it easier to write code that uses these threads incorrectly, and which will cause subtle threading issues. I realize production code doesn't benefit from these protections, but DCHECKs serve as a valuable 'defense in depth' of expressing the API conditions and guards against accidental API misuse during development, preventing bugs from making it to production.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Francois]

Calls to MessageLoopForUI::current()->PostTask/PostDelayedTask/DeleteSoon/ReleaseSoon have to be migrated because they are deprecated. However, since the UI thread won’t be migrated to base/task_scheduler in the near future, reviewers can ask me to migrate these calls to MessageLoopForUI::current()->task_runner()->PostTask/… instead of ThreadTaskRunnerHandle::Get()->PostTask/...

Calls to MessageLoopForIO::current()->PostTask/… also have to be migrated because they are deprecated. Before threads running a MessageLoopForIO are migrated to base/task_scheduler, we will introduce an async I/O API that can be used from any thread (work in progress). Therefore, there will be no reason to check that the current thread supports async I/O.

To verify that tasks don’t run at the same time (thread safety):

• SequenceChecker

To verify that tasks run on the same thread (thread affinity):

• ThreadChecker

To verify that a task runs on a given named thread:

• DCHECK_CURRENTLY_ON()

• Code that cannot access DCHECK_CURRENTLY_ON() shouldn’t know about Chrome threads, except maybe about the UI thread. DCHECK(MessageLoopForUI::IsCurrent() can be used to verify that code runs on the UI thread, but it is misleading because there are other threads running MessageLoopForUI.

[Ryan Sleevi]

I don't believe this addresses my concerns. Saying they have to be migrated because they're deprecated, but not having a clear direction about how to do the same, is not a good thing. Similarly, globally refactoring code to remove defensive checks/asserting on preconditions makes it more likely to introduce bugs.

Unrelatedly, your proposed work in progress API only addresses the POSIX case. What about the case of Windows' RegisterIOHandler/WaitForIOCompletion?

Concretely: You're already migrating calls to MessageLoop[ForUI/ForIO]->current()->PostTask, as evidenced by the linked CLs. In effect, every one of those PostTask calls you're migrating has concrete side-effects in developer builds, which is that you're removing the DCHECK(MessageLoopForIO::IsCurrent()) calls implicit in each of those PostTasks (specifically, https://cs.chromium.org/chromium/src/base/message_loop/message_loop.h?rcl=0&l=629 )

If you were just changing MessageLoop::current()->* calls, I wouldn't be concerned, but my point is that your proposal (change MessageLoop::current()) calls is not matched by what you're actually doing (changing MessageLoopForIO::current()/MessageLoopForUI::current() calls), and I was pointing out that it does have side-effects that makes it easier to introduce bugs. I'm not talking about the migration of threads running MessageLoopForUI/IO calls, but how those threads are interacted with. The DCHECKs serve to assert that conditions are sane - and your changes are removing those asserts.

If your plan is only to convert MessageLoop::current() calls, I totally agree. But your cleanup script does more than that, it has side effects, and I don't think those side-effects are desirable until we add similar defensive assertions to guard against accidental programmer mistakes, which, when dealing with threading code about to get significantly more complex due to the scheduler changes, will be far easier to make.

[Francois]

I understand your concerns about removing DCHECKs. I'll wait until we have an equally robust alternative before migrating more MessageLoop[ForUI/ForIO]::current() calls. I'll continue to migrate MessageLoop::current() calls in the meantime.

Fortunately, only one MessageLoopForIO::current() call site has been migrated so far https://codereview.chromium.org/2037853002/. I have reverted this change. I will also revert the MessageLoopForUI::current() migrations that occurred as part of https://codereview.chromium.org/2033753002/

[François]

Calls to deprecated MessageLoop methods have been removed from files that compile on Mac and the methods have been disabled for this platform using #ifdefs. The same process will be repeated for all platforms in the next days.

[Thiemo Nagel]

François, may I kindly ask you to bring the developer docs up to date with your change?

https://www.chromium.org/developers/design-documents/threading

Thank you,

Thiemo

[Gabriel Charette]

Indeed :), we happen to be in the process of re-writing these docs right now! (we were waiting on a few things to be in place first to only redo them once; i.e. without a transitional phase where the new recommendations don't work in every corner of the codebase)