"if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {"
Pavel Feldman
Ryan Sleevi
> --
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev
Right, in general, you should design your classes with explicit and well specified threading guarantees, and it should be documented what those are.
As willchan@ has ranted many times, the best design for a class is to have its external interfaces live on a single thread. If there are multiple threads involve, you should try to hide that detail from others, such as through the use of an inner class that is managed exclusively by the parent.
The biggest reason for that, beyond the simplicity of modeling the thread interactions, is because lifetimes should be well defined and explicit whenever possible, and an object that lives on multiple threads by nature introduces significant complexity and ordering requirements.
A DCHECK - or, better (in most cases), a ThreadWatcher member - helps you programmatically enforce the threading restrictions of your class. Assuming your code is well tested and with good coverage, it should make it clear when someone tries to use your object while developing something that they're holding it wrong.
For free functions, it is more complicated. Many times, the incantation you mention is due to the objects they are interacting with having underspecified guarantees, and thus the hop out of fear, uncertainty, or an abundance of caution. For classes / events that you're interacting with that have well defined semantics, they should do exactly as you say - DCHECK the postconditions of the source event / preconditions of the method and then carry on.
If you find a class you're using doesn't have well defined semantics, or that you're unsure, or that you find you're getting events in a way that surprises you, those are all signs of bugs (documentation or implementation) and design smells in the class you're using, and the Googley/Chromey thing would be to fix/refactor/document that code, and then put strong assertions like DCHECKs in your code.
Kinuko Yasuda
On Dec 14, 2014 11:38 PM, "Pavel Feldman" <pfel...@chromium.org> wrote:
>
> I was recently reviewing the https://codereview.chromium.org/761923004/ and noticed that the following pattern was used extensively in the code:
>
> void DoSomething() {
> if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
> BrowserThread::PostTask(BrowserThread::UI,
> FROM_HERE,
> base::Bind(DoSomething);
> return;
> }
> ...
> }
Ryan Sleevi
So, while readability is certainly subjective, I tend to find it easier to clearly call out in the style of 1.
DoSomethingOnIOThread(...) {
DCHECK(CurrentlyOn(IO));
PostTask(UI, FROM_HERE, base::Bind(DoSomethingOnUIThread)));
}
DoSomethingOnUIThread(...) {
DCHECK(CurrentlyOn(UI));
...
}
Not only do I find this more readable, but it actually ensures that your tests will test the right thing. Depending on which test harness you're using, you may find CurrentlyOn(A) == CurrentlyOn(B) == true when testing, but false when in real code.
The above two-method pattern makes it explicit that there should always be a PostTask yield in between, which can help shake out the subtle lifetime bugs inevitably introduced in multithreaded code.
The only time I recommend the if (!CurrentlyOn(..)) pattern is when you explicitly want to leverage this fact. For example, on Linux, our NSS TLS sockets are multithreaded, due to library limitations, whereas on Windows/Mac, they were single threaded. The if pattern helped to make this code easier to flow for single and multithreaded code, but at the cost of complexity.
To ensure you're always fully testing the right behaviors, I recommend the two-function approach. It is the least likely to have subtle surprises.