base::Bind and capturing |this|, is it possible?

[Gabriel Charette]

Hello fellow devs,

I'm refactoring some tests which are calling classes that expect to run on a given task runner. Currently all is "fine" because all TestMockTimeTaskRunners are "running" on main thread (i.e. RunsTasksOnCurrentThread() returns true for it). But I'd like to separate that (so that ThreadTaskRunnerHandle et al. provide the right runtime context when sharing the main thread in unit tests). As such test code that needs to run in another context would need to do something like:

  net_test_task_runner_->PostTask(FROM_HERE, base::Bind([this]() -> void {

    AFixtureMethodBoundToThisPtr();

    (and more test code)

  });

  net_test_task_runner_->RunUntilIdle();

but when I try to bind [this] or [&] into a lambda I get the following error, any idea? Is this supposed to work? Clearly this is a safe binding by reference because I execute the lambda synchronously right after.

D:\src\chrome\src\base\bind_internal.h(372,16):  error: implicit instantiation of undefined template 'base::internal::FunctorTraits<(lambda at ../../net/http/http_server_properties_manager_unittest.cc:874:34), void>'

      typename FunctorTraits<typename std::decay<Functor>::type>::RunType;

               ^

D:\src\chrome\src\base\bind_internal.h(538,1):  note: in instantiation of template class 'base::internal::MakeUnboundRunTypeImpl<(lambda at ../../net/http/http_server_properties_manager_unittest.cc:874:34)>' requested here

using MakeUnboundRunType =

^

D:\src\chrome\src\base\bind.h(75,17):  note: in instantiation of template type alias 'MakeUnboundRunType' requested here

inline Callback<MakeUnboundRunType<Functor, Args...>>

                ^

D:\src\chrome\src\net\http\http_server_properties_manager_unittest.cc(874,23):  note: while substituting deduced template arguments into function template 'Bind' [with Functor = (lambda at ../../net/http/http_server_properties_manager_unittest.cc:874:34), Args = <>]

  base::Closure foo = base::Bind([this]() -> void {

                      ^

D:\src\chrome\src\base\bind_internal.h(134,8):  note: template is declared here

struct FunctorTraits;

[Jeremy Roman]

No, it's not. A lambda that captures in this way isn't supported by base::Bind, because it would capture state in the lambda itself, bypassing the Chromium-specific logic base::Bind has.

Instead, you should use a captureless lambda (which decays to a function pointer), and bind anything you wish with base::Bind:

  net_test_task_runner_->PostTask(FROM_HERE, base::Bind([](WhateverTheTypeOfThisIs* self) {

    self->AFixtureMethodBoundToThisPtr();

    (and more test code)

  }, base::Unretained(this));

  net_test_task_runner_->RunUntilIdle();

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

[Lucas Gadani]

I've asked base owners before for this feature, and the problem is that we can't guarantee lifetime when binding this way.

There's a workaround you can do by binding as the first argument:

base::Bind([](ThisPtr* this_) { }, this);

I believe this has some drawbacks regarding the task scheduler, but it does work.

--

[Peter Kasting]

On Mon, Feb 13, 2017 at 1:37 PM, Jeremy Roman <jbr...@chromium.org> wrote:

Instead, you should use a captureless lambda (which decays to a function pointer), and bind anything you wish with base::Bind:

  net_test_task_runner_->PostTask(FROM_HERE, base::Bind([](WhateverTheTypeOfThisIs* self) {

    self->AFixtureMethodBoundToThisPtr();

    (and more test code)

  }, base::Unretained(this));

  net_test_task_runner_->RunUntilIdle();

This.  See also https://groups.google.com/a/chromium.org/forum/#!topic/cxx/QxjsPELDYdQ and the notes in the "lambda expressions" section of https://chromium-cpp.appspot.com/ (which links there).

PK 

[Gabriel Charette]

Re. binding |this| into an explicit |self| pointer : right that works it just makes the whole paradigm I'm trying to introduce much more cumbersome than desired :(. I want it to be easy to read the test in one block knowing that various blocks execute in various task contexts.

Re. can't guarantee lifetime : it's true that one has to be careful with captures and lifetime but this case should be fine per execution happening synchronously right after binding.

Re. notes linked by PK : right, read those before posting this but they don't mention this unless I missed it (from my reading they merely say what I already know : that capturing is risky if not done properly).

[Peter Kasting]

On Mon, Feb 13, 2017 at 1:51 PM, Gabriel Charette <g...@chromium.org> wrote:

Re. can't guarantee lifetime : it's true that one has to be careful with captures and lifetime but this case should be fine per execution happening synchronously right after binding.

That's basically what the explicit "base::Unretained(this)" is saying: you're manually guaranteeing that |this| will be kept alive through the life of the task.

The whole point of the machinery here is to force people who post tasks to go through the explicit method of annotating lifetime, instead of using captures, so that both the author and future readers think about the potential issues.  The drawback is that it's more verbose, which is a tradeoff we're willing to make.

Re. notes linked by PK : right, read those before posting this but they don't mention this unless I missed it (from my reading they merely say what I already know : that capturing is risky if not done properly).

It's true that what you're doing is safe, but to allow Bind() to accept it would require also accepting things that aren't safe.  There's no way Bind() can know that you're going to have a RunUntilIdle() call after the Bind() call, for example, and without that this could easily be a lifetime bug.

PK

[Daniel Cheng]

base::Bind intentionally doesn't work with capturing lambdas for the reasons already stated: base::Bind is expected to execute asynchronously, so the bound callback needs to carefully consider lifetimes of objects. base::Bind tries to force this by requiring annotations on the receiver pointer and providing WeakPtr cancellation semantics, while lambdas don't have any conventions.

The problem is that this particular bit of code is mixing capturing lambdas with a Bind: as PK said, there's no way that Bind() would know that this is safe. It's unfortunate, but I think it's a fair tradeoff for increased safety around object lifetimes: in fact, even with base::Bind(), we still have more use-after-frees than I'd like...

Daniel

[Gabriel Charette]

I'm all for forcing explicit markups so that devs are forced to make a call and so that it acts as documentation to reviewer and reader.

But could we have base::BindWithCapture() or something as an explicit thing for people that know what they are doing?

[Daniel Cheng]

On Mon, Feb 13, 2017 at 2:17 PM Gabriel Charette <g...@chromium.org> wrote:

I'm all for forcing explicit markups so that devs are forced to make a call and so that it acts as documentation to reviewer and reader.

But could we have base::BindWithCapture() or something as an explicit thing for people that know what they are doing?

base::Unretained() is a similar construct (I know what I'm doing and I promise it's safe), but it's frequently used with no comment as to why it's safe: I looked at the first 10 files on codesearch that used base::Unretained() and found one comment explaining why Unretained is safe. I think introducing BindWithCapture() would have the same issues.

In the end, it just doesn't seem like a common enough use case, nor one that should be encouraged, given how complex Bind() already is.

Daniel

[Dmitry Skiba]

Hmm, I wonder if that instantiation can explicitly be implement to give nice and clean static_assertion instead of generic implicit instantiation of undefined template error.

---
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+unsubscribe@chromium.org.

[Rachel Blum]

 I looked at the first 10 files on codesearch

We should have straightforward documentation on these issues. And we actually do - it's in docs/callback.md. 

Sidebar, break off a thread if you want to debate it: Why it's in a separate docs folder instead right next to the code it explains (so you might remember to update the docs when you update the code), or why it's callback.md instead of bind.md, or why bind.h points you to callback.h points you to callback.md is a mystery to me, though.

[Gabriel Charette]

On Mon, Feb 13, 2017 at 5:29 PM Daniel Cheng <dch...@chromium.org> wrote:

On Mon, Feb 13, 2017 at 2:17 PM Gabriel Charette <g...@chromium.org> wrote:

I'm all for forcing explicit markups so that devs are forced to make a call and so that it acts as documentation to reviewer and reader.

But could we have base::BindWithCapture() or something as an explicit thing for people that know what they are doing?

base::Unretained() is a similar construct (I know what I'm doing and I promise it's safe), but it's frequently used with no comment as to why it's safe: I looked at the first 10 files on codesearch that used base::Unretained() and found one comment explaining why Unretained is safe. I think introducing BindWithCapture() would have the same issues.

I agree that base::Unretained() should always be accompanied by a comment stating why it's safe (unless it's blatantly obvious -- e.g. in unit tests binding the test fixture).

base::BindWithCapture() is essentially the same as base::Unretained(), i.e. it doesn't force documentation but it's at least an explicit name that requires dev/reviewer to think about it and documents it for future readers. I don't see why one is okay but not the other.

 

In the end, it just doesn't seem like a common enough use case, nor one that should be encouraged, given how complex Bind() already is.

I'm hoping to move unit testing to a world where every task runner runs on the main thread but ThreadTaskRunnerHandle/RunsTasksOnCurrentThread/etc. respond to being in its scope (not being on main thread) -- with escape hatches for the few tests that absolutely need async. That way we can easily run multi-threaded tests on the main thread (and deterministically run phases of the test as desired + mock tick clocks, yay!).

To achieve this it needs to be easy/pretty to have test code run in the context of one of those runners and being able to capture |this| is essential to avoid this paradigm being too verbose to be nice to use.

[dan...@chromium.org]

On Tue, Feb 14, 2017 at 11:08 AM, Gabriel Charette <g...@chromium.org> wrote:

On Mon, Feb 13, 2017 at 5:29 PM Daniel Cheng <dch...@chromium.org> wrote:

On Mon, Feb 13, 2017 at 2:17 PM Gabriel Charette <g...@chromium.org> wrote:

I'm all for forcing explicit markups so that devs are forced to make a call and so that it acts as documentation to reviewer and reader.

But could we have base::BindWithCapture() or something as an explicit thing for people that know what they are doing?

base::Unretained() is a similar construct (I know what I'm doing and I promise it's safe), but it's frequently used with no comment as to why it's safe: I looked at the first 10 files on codesearch that used base::Unretained() and found one comment explaining why Unretained is safe. I think introducing BindWithCapture() would have the same issues.

I agree that base::Unretained() should always be accompanied by a comment stating why it's safe (unless it's blatantly obvious -- e.g. in unit tests binding the test fixture).

base::BindWithCapture() is essentially the same as base::Unretained(), i.e. it doesn't force documentation but it's at least an explicit name that requires dev/reviewer to think about it and documents it for future readers. I don't see why one is okay but not the other.

I think we don't need two ways to do this, and Unretained is more general, and already used everywhere.

 

 

In the end, it just doesn't seem like a common enough use case, nor one that should be encouraged, given how complex Bind() already is.

I'm hoping to move unit testing to a world where every task runner runs on the main thread but ThreadTaskRunnerHandle/RunsTasksOnCurrentThread/etc. respond to being in its scope (not being on main thread) -- with escape hatches for the few tests that absolutely need async. That way we can easily run multi-threaded tests on the main thread (and deterministically run phases of the test as desired + mock tick clocks, yay!).

To achieve this it needs to be easy/pretty to have test code run in the context of one of those runners and being able to capture |this| is essential to avoid this paradigm being too verbose to be nice to use.

Why doesn't a helper function that produces a Callback and binds this into it or something like that work?

[Gabriel Charette]

On Tue, Feb 14, 2017 at 11:12 AM <dan...@chromium.org> wrote:

On Tue, Feb 14, 2017 at 11:08 AM, Gabriel Charette <g...@chromium.org> wrote:

On Mon, Feb 13, 2017 at 5:29 PM Daniel Cheng <dch...@chromium.org> wrote:

On Mon, Feb 13, 2017 at 2:17 PM Gabriel Charette <g...@chromium.org> wrote:

I'm all for forcing explicit markups so that devs are forced to make a call and so that it acts as documentation to reviewer and reader.

But could we have base::BindWithCapture() or something as an explicit thing for people that know what they are doing?

base::Unretained() is a similar construct (I know what I'm doing and I promise it's safe), but it's frequently used with no comment as to why it's safe: I looked at the first 10 files on codesearch that used base::Unretained() and found one comment explaining why Unretained is safe. I think introducing BindWithCapture() would have the same issues.

I agree that base::Unretained() should always be accompanied by a comment stating why it's safe (unless it's blatantly obvious -- e.g. in unit tests binding the test fixture).

base::BindWithCapture() is essentially the same as base::Unretained(), i.e. it doesn't force documentation but it's at least an explicit name that requires dev/reviewer to think about it and documents it for future readers. I don't see why one is okay but not the other.

I think we don't need two ways to do this, and Unretained is more general, and already used everywhere.

 

 

In the end, it just doesn't seem like a common enough use case, nor one that should be encouraged, given how complex Bind() already is.

I'm hoping to move unit testing to a world where every task runner runs on the main thread but ThreadTaskRunnerHandle/RunsTasksOnCurrentThread/etc. respond to being in its scope (not being on main thread) -- with escape hatches for the few tests that absolutely need async. That way we can easily run multi-threaded tests on the main thread (and deterministically run phases of the test as desired + mock tick clocks, yay!).

To achieve this it needs to be easy/pretty to have test code run in the context of one of those runners and being able to capture |this| is essential to avoid this paradigm being too verbose to be nice to use.

Why doesn't a helper function that produces a Callback and binds this into it or something like that work?

Consider the HttpServerPropertiesManagerTest, ConfirmAlternativeService test.

It's 50 lines of code that should all run on the |net_test_task_runner_| (all of the methods invoked DCHECK(net_test_task_runner_->RunsOnCurrentThread()). Currently this happens to pass despite this code running in the main thread's context because TestMockTimeTaskRunner::RunsOnCurrentThread() is looser than I'd like (it checks the thread id instead of whether it's truly currently executing something in its context).

This is a problem because it means that if any of the tested code uses ThreadTaskRunnerHandle::Get(), it'll get the main thread's task runner instead of |net_test_task_runner_| (which is wrong given it just verified it's running on |net_test_task_runner_| -- and blocks https://codereview.chromium.org/2491613004/).

A helper function either has to bind |this| inline (and become verbose) or be a method on the test fixture (which makes the test hard to read -- its body essentially moves to the test fixture...).

The cleanest test code would be to have the test code inline in the test body, merely encapsulated in a lambda that captures [this].

I expect this test pattern to be more and more common as we move more and more things to independent sequences and services and having a nice test paradigm to follow here will help make our tests (and thus codebase) healthier.

Gab

[Jeremy Roman]

On Tue, Feb 14, 2017 at 11:36 AM, Gabriel Charette <g...@chromium.org> wrote:

On Tue, Feb 14, 2017 at 11:12 AM <dan...@chromium.org> wrote:

On Tue, Feb 14, 2017 at 11:08 AM, Gabriel Charette <g...@chromium.org> wrote:

On Mon, Feb 13, 2017 at 5:29 PM Daniel Cheng <dch...@chromium.org> wrote:

On Mon, Feb 13, 2017 at 2:17 PM Gabriel Charette <g...@chromium.org> wrote:

I'm all for forcing explicit markups so that devs are forced to make a call and so that it acts as documentation to reviewer and reader.

But could we have base::BindWithCapture() or something as an explicit thing for people that know what they are doing?

base::Unretained() is a similar construct (I know what I'm doing and I promise it's safe), but it's frequently used with no comment as to why it's safe: I looked at the first 10 files on codesearch that used base::Unretained() and found one comment explaining why Unretained is safe. I think introducing BindWithCapture() would have the same issues.

I agree that base::Unretained() should always be accompanied by a comment stating why it's safe (unless it's blatantly obvious -- e.g. in unit tests binding the test fixture).

base::BindWithCapture() is essentially the same as base::Unretained(), i.e. it doesn't force documentation but it's at least an explicit name that requires dev/reviewer to think about it and documents it for future readers. I don't see why one is okay but not the other.

I think we don't need two ways to do this, and Unretained is more general, and already used everywhere.

 

 

In the end, it just doesn't seem like a common enough use case, nor one that should be encouraged, given how complex Bind() already is.

I'm hoping to move unit testing to a world where every task runner runs on the main thread but ThreadTaskRunnerHandle/RunsTasksOnCurrentThread/etc. respond to being in its scope (not being on main thread) -- with escape hatches for the few tests that absolutely need async. That way we can easily run multi-threaded tests on the main thread (and deterministically run phases of the test as desired + mock tick clocks, yay!).

To achieve this it needs to be easy/pretty to have test code run in the context of one of those runners and being able to capture |this| is essential to avoid this paradigm being too verbose to be nice to use.

Why doesn't a helper function that produces a Callback and binds this into it or something like that work?

Consider the HttpServerPropertiesManagerTest, ConfirmAlternativeService test.

It's 50 lines of code that should all run on the |net_test_task_runner_| (all of the methods invoked DCHECK(net_test_task_runner_->RunsOnCurrentThread()). Currently this happens to pass despite this code running in the main thread's context because TestMockTimeTaskRunner::RunsOnCurrentThread() is looser than I'd like (it checks the thread id instead of whether it's truly currently executing something in its context).

This is a problem because it means that if any of the tested code uses ThreadTaskRunnerHandle::Get(), it'll get the main thread's task runner instead of |net_test_task_runner_| (which is wrong given it just verified it's running on |net_test_task_runner_| -- and blocks https://codereview.chromium.org/2491613004/).

A helper function either has to bind |this| inline (and become verbose) or be a method on the test fixture (which makes the test hard to read -- its body essentially moves to the test fixture...).

The cleanest test code would be to have the test code inline in the test body, merely encapsulated in a lambda that captures [this].

I expect this test pattern to be more and more common as we move more and more things to independent sequences and services and having a nice test paradigm to follow here will help make our tests (and thus codebase) healthier.

Gab

One tactic that I think might be in the spirit of the style guide, though it's a little quirky (though I'd be curious what danakj/dcheng think):

auto lambda = [this](...) {

  // ...

};

base::WeakPtrFactory<decltype(lambda)> weak_factory(&lambda);

net_test_task_runner_->PostTask(FROM_HERE, base::Bind(&lambda, &decltype(lambda)::operator()));

net_test_task_runner_->RunUntilIdle();

This keeps the lambda capture on the stack, and ensures it isn't invoked after destruction by using a weak pointer. The pattern could be extracted into a helper class (if we're okay with moving the lambda):

template <typename F>

class WeakLambda {

 public:

  WeakLambda(F f) : f_(std::move(f)), weak_factory_(&f_) {}

  base::Closure Bind() const { return base::Bind(&f_, &F::operator()); }

 private:

  F f_;

  base::WeakPtrFactory<F> weak_factory_;

};

template <typename F> WeakLambda<F> MakeWeakLambda(F f) { return WeakLambda<F>(std::move(f)); }

auto lambda = MakeWeakLambda([this](...) {

  // ...

});

net_test_task_runner_->PostTask(FROM_HERE, lambda.Bind());

net_test_task_runner_->RunUntilIdle();

Or extracting even further:

template <typename F>

void PostWeakLambdaAndRunUntilIdle(base::TaskRunner* runner, const tracked_objects::Location& from_here, F f) {

  auto lambda = MakeWeakLambda(std::move(f));

  runner->PostTask(from_here, lambda.Bind());

  runner->RunUntilIdle();

}

PostWeakLambdaAndRunUntilIdle(net_task_runner_, FROM_HERE, [this](...) {

  // ...

});

But I digress. Even in test code, I'm a little cautious about adding more patterns that are actually unsafe, because it's common to look at test code for inspiration when writing production code, and it might not be obvious why we don't allow lambda capture on the heap.

[dan...@chromium.org]

On Tue, Feb 14, 2017 at 11:36 AM, Gabriel Charette <g...@chromium.org> wrote:

On Tue, Feb 14, 2017 at 11:12 AM <dan...@chromium.org> wrote:

On Tue, Feb 14, 2017 at 11:08 AM, Gabriel Charette <g...@chromium.org> wrote:

On Mon, Feb 13, 2017 at 5:29 PM Daniel Cheng <dch...@chromium.org> wrote:

On Mon, Feb 13, 2017 at 2:17 PM Gabriel Charette <g...@chromium.org> wrote:

I'm all for forcing explicit markups so that devs are forced to make a call and so that it acts as documentation to reviewer and reader.

But could we have base::BindWithCapture() or something as an explicit thing for people that know what they are doing?

base::Unretained() is a similar construct (I know what I'm doing and I promise it's safe), but it's frequently used with no comment as to why it's safe: I looked at the first 10 files on codesearch that used base::Unretained() and found one comment explaining why Unretained is safe. I think introducing BindWithCapture() would have the same issues.

I agree that base::Unretained() should always be accompanied by a comment stating why it's safe (unless it's blatantly obvious -- e.g. in unit tests binding the test fixture).

base::BindWithCapture() is essentially the same as base::Unretained(), i.e. it doesn't force documentation but it's at least an explicit name that requires dev/reviewer to think about it and documents it for future readers. I don't see why one is okay but not the other.

I think we don't need two ways to do this, and Unretained is more general, and already used everywhere.

 

 

In the end, it just doesn't seem like a common enough use case, nor one that should be encouraged, given how complex Bind() already is.

I'm hoping to move unit testing to a world where every task runner runs on the main thread but ThreadTaskRunnerHandle/RunsTasksOnCurrentThread/etc. respond to being in its scope (not being on main thread) -- with escape hatches for the few tests that absolutely need async. That way we can easily run multi-threaded tests on the main thread (and deterministically run phases of the test as desired + mock tick clocks, yay!).

To achieve this it needs to be easy/pretty to have test code run in the context of one of those runners and being able to capture |this| is essential to avoid this paradigm being too verbose to be nice to use.

Why doesn't a helper function that produces a Callback and binds this into it or something like that work?

Consider the HttpServerPropertiesManagerTest, ConfirmAlternativeService test.

It's 50 lines of code that should all run on the |net_test_task_runner_| (all of the methods invoked DCHECK(net_test_task_runner_->RunsOnCurrentThread()). Currently this happens to pass despite this code running in the main thread's context because TestMockTimeTaskRunner::RunsOnCurrentThread() is looser than I'd like (it checks the thread id instead of whether it's truly currently executing something in its context).

This is a problem because it means that if any of the tested code uses ThreadTaskRunnerHandle::Get(), it'll get the main thread's task runner instead of |net_test_task_runner_| (which is wrong given it just verified it's running on |net_test_task_runner_| -- and blocks https://codereview.chromium.org/2491613004/).

A helper function either has to bind |this| inline (and become verbose) or be a method on the test fixture (which makes the test hard to read -- its body essentially moves to the test fixture...).

The cleanest test code would be to have the test code inline in the test body, merely encapsulated in a lambda that captures [this].

I expect this test pattern to be more and more common as we move more and more things to independent sequences and services and having a nice test paradigm to follow here will help make our tests (and thus codebase) healthier.

I think there's 2 pretty reasonable ways to do this that don't involve binding this into a lambda:

1. Move the code into a method

HttpServerPropertiesManagerTest::RunConfirmAlternativeService(WaitableEvent* event) {
  // Move all the test code here..

  event->Signal();

}

TEST_P(HttpServerPropertiesManagerTest, ConfirmAlternativeService) {

  WaitableEvent event;

  auto callback = base::Bind(&HttpServerPropertiesManagerTest::RunConfirmAlternativeService, base::Unretained(this), &event);

  net_test_task_runner_->PostTask(std::move(callback));

  event.Wait();

}

2. Bind the lambda without this.

TEST_P(HttpServerPropertiesManagerTest, ConfirmAlternativeService) {

  auto lambda = [](HttpServerPropertiesManagerTest* self, WaitableEvent* event) {

    // Move all the test code here.. and use public instead of private members.

    event->Signal();

  }

  WaitableEvent event;

  auto callback = base::Bind(lambda, base::Unretained(this), &event);

  net_test_task_runner_->PostTask(std::move(callback));

  event.Wait();

}

Neither of these seem particularly painful to me. Do they do you?

[Gabriel Charette]

On Tue, Feb 14, 2017 at 12:09 PM <dan...@chromium.org> wrote:

On Tue, Feb 14, 2017 at 11:36 AM, Gabriel Charette <g...@chromium.org> wrote:

On Tue, Feb 14, 2017 at 11:12 AM <dan...@chromium.org> wrote:

On Tue, Feb 14, 2017 at 11:08 AM, Gabriel Charette <g...@chromium.org> wrote:

On Mon, Feb 13, 2017 at 5:29 PM Daniel Cheng <dch...@chromium.org> wrote:

On Mon, Feb 13, 2017 at 2:17 PM Gabriel Charette <g...@chromium.org> wrote:

I'm all for forcing explicit markups so that devs are forced to make a call and so that it acts as documentation to reviewer and reader.

But could we have base::BindWithCapture() or something as an explicit thing for people that know what they are doing?

base::Unretained() is a similar construct (I know what I'm doing and I promise it's safe), but it's frequently used with no comment as to why it's safe: I looked at the first 10 files on codesearch that used base::Unretained() and found one comment explaining why Unretained is safe. I think introducing BindWithCapture() would have the same issues.

I agree that base::Unretained() should always be accompanied by a comment stating why it's safe (unless it's blatantly obvious -- e.g. in unit tests binding the test fixture).

base::BindWithCapture() is essentially the same as base::Unretained(), i.e. it doesn't force documentation but it's at least an explicit name that requires dev/reviewer to think about it and documents it for future readers. I don't see why one is okay but not the other.

I think we don't need two ways to do this, and Unretained is more general, and already used everywhere.

 

 

In the end, it just doesn't seem like a common enough use case, nor one that should be encouraged, given how complex Bind() already is.

I'm hoping to move unit testing to a world where every task runner runs on the main thread but ThreadTaskRunnerHandle/RunsTasksOnCurrentThread/etc. respond to being in its scope (not being on main thread) -- with escape hatches for the few tests that absolutely need async. That way we can easily run multi-threaded tests on the main thread (and deterministically run phases of the test as desired + mock tick clocks, yay!).

To achieve this it needs to be easy/pretty to have test code run in the context of one of those runners and being able to capture |this| is essential to avoid this paradigm being too verbose to be nice to use.

Why doesn't a helper function that produces a Callback and binds this into it or something like that work?

Consider the HttpServerPropertiesManagerTest, ConfirmAlternativeService test.

It's 50 lines of code that should all run on the |net_test_task_runner_| (all of the methods invoked DCHECK(net_test_task_runner_->RunsOnCurrentThread()). Currently this happens to pass despite this code running in the main thread's context because TestMockTimeTaskRunner::RunsOnCurrentThread() is looser than I'd like (it checks the thread id instead of whether it's truly currently executing something in its context).

This is a problem because it means that if any of the tested code uses ThreadTaskRunnerHandle::Get(), it'll get the main thread's task runner instead of |net_test_task_runner_| (which is wrong given it just verified it's running on |net_test_task_runner_| -- and blocks https://codereview.chromium.org/2491613004/).

A helper function either has to bind |this| inline (and become verbose) or be a method on the test fixture (which makes the test hard to read -- its body essentially moves to the test fixture...).

The cleanest test code would be to have the test code inline in the test body, merely encapsulated in a lambda that captures [this].

I expect this test pattern to be more and more common as we move more and more things to independent sequences and services and having a nice test paradigm to follow here will help make our tests (and thus codebase) healthier.

I think there's 2 pretty reasonable ways to do this that don't involve binding this into a lambda:

These are the two I mentioned above as well and don't like:

 

1. Move the code into a method

HttpServerPropertiesManagerTest::RunConfirmAlternativeService(WaitableEvent* event) {
  // Move all the test code here..

  event->Signal();

}

TEST_P(HttpServerPropertiesManagerTest, ConfirmAlternativeService) {

  WaitableEvent event;

  auto callback = base::Bind(&HttpServerPropertiesManagerTest::RunConfirmAlternativeService, base::Unretained(this), &event);

  net_test_task_runner_->PostTask(std::move(callback));

  event.Wait();

}

This makes the test less readable by (1) having to look at a helper and (2) convoluting the test fixture with individual test bodies.

2. Bind the lambda without this.

TEST_P(HttpServerPropertiesManagerTest, ConfirmAlternativeService) {

  auto lambda = [](HttpServerPropertiesManagerTest* self, WaitableEvent* event) {

    // Move all the test code here.. and use public instead of private members.

    event->Signal();

  }

  WaitableEvent event;

  auto callback = base::Bind(lambda, base::Unretained(this), &event);

  net_test_task_runner_->PostTask(std::move(callback));

  event.Wait();

}

This result in a lot of "self->" and forces public members (currently that test happens to use a lot of protected method for testing -- perhaps a bad paradigm on its own but one I was trying to avoid addressing).

 

Neither of these seem particularly painful to me. Do they do you?

Yes. In isolation they're not bad but if this becomes the recommended paradigm to write tests as more things are sequenced it will be cumbersome and make our "my codebase doesn't have unnecessary complexity" even worse IMO.

I just thought of a third-way which would be fairly nice:

// Returns an object in whose scope executed code is seen as running in

// this TestMockTimeTaskRunner's context (i.e. ThreadTaskRunnerHandle, RunsTasksOnCurrentThread, etc.).

std::unique_ptr<TestMockTimeTaskRunner::Context> TestMockTimeTaskRunner::EnterRuntimeContext();

Then I can do what I want (run synchronously in the scope of that task runner without having to jump through hoops) -- this is actually even cleaner than bind lambda+RunUntilIdle().

I'll go ahead and implement this.

Cheers,

Gab

[dan...@chromium.org]

The code is moving into a helper method/function either way. In this case it's right above the TEST_P line instead of right below it, which seems innocuous to me. It's true you have to add a defn for RunConfirmAlternativeService() to the class, but that doesn't appear terrible to me for a test suite either.

 

2. Bind the lambda without this.

TEST_P(HttpServerPropertiesManagerTest, ConfirmAlternativeService) {

  auto lambda = [](HttpServerPropertiesManagerTest* self, WaitableEvent* event) {

    // Move all the test code here.. and use public instead of private members.

    event->Signal();

  }

  WaitableEvent event;

  auto callback = base::Bind(lambda, base::Unretained(this), &event);

  net_test_task_runner_->PostTask(std::move(callback));

  event.Wait();

}

This result in a lot of "self->" and forces public members (currently that test happens to use a lot of protected method for testing -- perhaps a bad paradigm on its own but one I was trying to avoid addressing).

This isn't my favorite either because of all the self->, but making protected methods public in a test harness seems pretty okay.

 

 

Neither of these seem particularly painful to me. Do they do you?

Yes. In isolation they're not bad but if this becomes the recommended paradigm to write tests as more things are sequenced it will be cumbersome and make our "my codebase doesn't have unnecessary complexity" even worse IMO.

I guess I don't agree that these equate to complexity, considering the proposal was to move the body of the test onto another thread, and the complexity that implies.

 

I just thought of a third-way which would be fairly nice:

// Returns an object in whose scope executed code is seen as running in

// this TestMockTimeTaskRunner's context (i.e. ThreadTaskRunnerHandle, RunsTasksOnCurrentThread, etc.).

std::unique_ptr<TestMockTimeTaskRunner::Context> TestMockTimeTaskRunner::EnterRuntimeContext();

Then I can do what I want (run synchronously in the scope of that task runner without having to jump through hoops) -- this is actually even cleaner than bind lambda+RunUntilIdle().

I'll go ahead and implement this.

But I'm glad you've got another solution. In this case it's avoiding the use of posting a method then?

[Gabriel Charette]

But then you end up with a fixture full of methods used only by one of their tests. That's what I find convoluted.

 

 

2. Bind the lambda without this.

TEST_P(HttpServerPropertiesManagerTest, ConfirmAlternativeService) {

  auto lambda = [](HttpServerPropertiesManagerTest* self, WaitableEvent* event) {

    // Move all the test code here.. and use public instead of private members.

    event->Signal();

  }

  WaitableEvent event;

  auto callback = base::Bind(lambda, base::Unretained(this), &event);

  net_test_task_runner_->PostTask(std::move(callback));

  event.Wait();

}

This result in a lot of "self->" and forces public members (currently that test happens to use a lot of protected method for testing -- perhaps a bad paradigm on its own but one I was trying to avoid addressing).

This isn't my favorite either because of all the self->, but making protected methods public in a test harness seems pretty okay.

Thinking about it some more, s/protected/public on test harness is fine (in this case that'd have to apply to members too which is against style guide -- but I guess we're already kind of saying that's ok already by having protected members in fixtures -- IMO that's fine). Overall it's mostly the |self| overhead I don't like.

 

 

 

Neither of these seem particularly painful to me. Do they do you?

Yes. In isolation they're not bad but if this becomes the recommended paradigm to write tests as more things are sequenced it will be cumbersome and make our "my codebase doesn't have unnecessary complexity" even worse IMO.

I guess I don't agree that these equate to complexity, considering the proposal was to move the body of the test onto another thread, and the complexity that implies.

Note: not "onto another thread", merely "onto another task runner" (which runs on synchronously on the main thread.

 

 

I just thought of a third-way which would be fairly nice:

// Returns an object in whose scope executed code is seen as running in

// this TestMockTimeTaskRunner's context (i.e. ThreadTaskRunnerHandle, RunsTasksOnCurrentThread, etc.).

std::unique_ptr<TestMockTimeTaskRunner::Context> TestMockTimeTaskRunner::EnterRuntimeContext();

Then I can do what I want (run synchronously in the scope of that task runner without having to jump through hoops) -- this is actually even cleaner than bind lambda+RunUntilIdle().

I'll go ahead and implement this.

But I'm glad you've got another solution. In this case it's avoiding the use of posting a method then?

But yeah overall that's a simpler solution to get things running in the scope of a mock task runner. Indeed it avoids posting a task at all :).

Thanks for the brainstorm everyone :)