MessageLoop::DeleteSoon

[Alok Priyadarshi]

I discovered that MessageLoop::DeleteSoon does not delete the object if the message loop is destroyed before it has a chance to run. This is because the MessageLoop destructor does not run the pending tasks, just pops them off the work queue.

Although the comment near the destructor seems to suggest that DeleteSoon might be run: "Clean up any unprocessed tasks, but take care: deleting a task could result in the addition of more tasks (e.g., via DeleteSoon)"

https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/message_loop.cc&sq=package:chromium&type=cs&l=153

I want to ensure that an object gets deleted when MessageLoop is destroyed. One of the ways I can think of is:

void DoNothing(const void* obj) {}

message_loop.PostTask(FROM_HERE, base::Bind(&DoNothing, base::Owned(obj));

Is there another way other than manually running the message loop? Or should DeleteSoon ensure that objects be destroyed?

[Dale Curtis]

Doesn't DeleteSoon return a bool indicating if it will delete the object or not? If it definitely won't the thread is dead and the best you can do is leak or destruct on a different thread.

- dale

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

[Alok Priyadarshi]

Ah yes - SequencedTaskRunner::DeleteSoon returns a bool - and that is what I am using (instead of MessageLoop::DeleteSoon in the original email).

But the returned bool is "maybe" and is always true unless the underlying message loop is already destroyed or being destroyed. In my case, it always returns true because the message loop outlives the object I am attempting to delete.

This is typically an issue for unittests where message-loop corresponds to the test main thread. When the test instance is destroyed, the message loop destructor does not run any of the posted tasks (including DeleteSoon tasks).

Perhaps we need a TestMessageLoop similar to TestBrowserThreadBundle that flushes the task queue for a clean unittest teardown?

[Max Bogue]

I believe this exact issue is the cause of http://crbug.com/588849 (flaky memory leaks in a test). I would love there to be a canonical solution!

---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Wez]

Seems that thread A using DeleteSoon to delete an object on thread B that is racey with thread B's shutting down is inherently flaky.

If DeleteSoon accepted a unique_ptr<> then at least it would ensure teardown of the object (as part of the queued Closure getting de-referenced & deleted on thread A); then callers would at least see thread-checker errors to point them to resolve the race-conditions in their code.

WDYT?

[Alok Priyadarshi]

On Mon, Apr 11, 2016 at 1:52 PM Wez <w...@chromium.org> wrote:

Seems that thread A using DeleteSoon to delete an object on thread B that is racey with thread B's shutting down is inherently flaky.

Agreed. Although IIUC this is not a problem when using a real base::Thread whose destructor ensures that all posted tasks actually run.

https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thread.cc&sq=package:chromium&type=cs&l=36&rcl=1460387938

I am almost convinced that this is only an issue with unittests that create a MessageLoop without a real thread backing it. I think creating a TestMessageLoop that on destruction, runs all posted tasks will be sufficient.

 

If DeleteSoon accepted a unique_ptr<> then at least it would ensure teardown of the object (as part of the queued Closure getting de-referenced & deleted on thread A); then callers would at least see thread-checker errors to point them to resolve the race-conditions in their code.

May be. Using a unique_ptr will break code using "friend base::DeleteHelper<Foo>" though.

[Alok Priyadarshi]

I just sent a patch to CQ that implements TestMessageLoop:

https://codereview.chromium.org/1876903005/

Please use it.