Add thread-checking to TransportFeedbackAdapter

Add thread-checking to TransportFeedbackAdapter.

BUG=None

[elad.alon_webrtc.org, 2017-02-23 14:05:16]

elad.alon@webrtc.org changed reviewers:
+ michaelt@webrtc.org, minyue@chromium.org, sprang@webrtc.org, stefan@webrtc.org

[elad.alon_webrtc.org, 2017-02-23 14:05:16]

Ready for review.

[minyue-webrtc, 2017-02-23 14:13:37]

minyue@webrtc.org changed reviewers:
+ minyue@webrtc.org

[minyue-webrtc, 2017-02-23 14:13:38]

I don't have enough background to judge. should it be thread checker or lock?

[elad.alon_webrtc.org, 2017-02-23 14:14:48]

On 2017/02/23 14:13:38, minyue-webrtc wrote:
> I don't have enough background to judge. should it be thread checker or lock?

Right now it's not being used, but if it ever does, the thread checker should
make sure it's accessed from the same thread that's updating the variable. If
anyone ever needs to access it from a different thread, he can use a (more
costly) lock.

[elad.alon_webrtc.org, 2017-02-23 14:16:49]

On 2017/02/23 14:14:48, elad.alon wrote:
> On 2017/02/23 14:13:38, minyue-webrtc wrote:
> > I don't have enough background to judge. should it be thread checker or
lock?
> 
> Right now it's not being used, but if it ever does, the thread checker should
> make sure it's accessed from the same thread that's updating the variable. If
> anyone ever needs to access it from a different thread, he can use a (more
> costly) lock.

Following some additional discussion with Stefan, btw, we're going to go with
thread-checking for the three possible threads that can access the object.

[minyue-webrtc, 2017-02-23 14:45:51]

https://codereview.webrtc.org/2710213003/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/transport_feedback_adapter.h (right):

https://codereview.webrtc.org/2710213003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/transport_feedback_adapter.h:76:
rtc::ThreadChecker last_packet_feedback_vector_thread_checker_;
one cannot bind a thread checker to a data.  Rather, it is to check whether a 
method is called on a correct thread. therefore, it is a bit awkward to call
this last_packet_feedback_vector_thread_checker_.

So I think it would be better to call this feedback_access_thread_checker_ or
similar.

But I like a lock even more.

[elad.alon_webrtc.org, 2017-02-23 14:52:01]

https://codereview.webrtc.org/2710213003/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/transport_feedback_adapter.h (right):

https://codereview.webrtc.org/2710213003/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/transport_feedback_adapter.h:76:
rtc::ThreadChecker last_packet_feedback_vector_thread_checker_;
On 2017/02/23 14:45:51, minyue-webrtc wrote:
> one cannot bind a thread checker to a data.  Rather, it is to check whether a 
> method is called on a correct thread. therefore, it is a bit awkward to call
> this last_packet_feedback_vector_thread_checker_.
> 
> So I think it would be better to call this feedback_access_thread_checker_ or
> similar.
> 
> But I like a lock even more.
> 

1. I'm introducing checkers now to all of the functions, as per the last
comment. They'll be named after their threads' function, not after the data they
protect.
2. Stefan preferred a thread checker. Either decision seems okay to me in
principle, but I prefer the checker, since I see no reason to incur the cost of
a lock when we only ever touch this from one thread.

[minyue-webrtc, 2017-02-23 15:05:18]

On 2017/02/23 14:52:01, elad.alon wrote:
>
https://codereview.webrtc.org/2710213003/diff/1/webrtc/modules/congestion_con...
> File webrtc/modules/congestion_controller/transport_feedback_adapter.h
(right):
> 
>
https://codereview.webrtc.org/2710213003/diff/1/webrtc/modules/congestion_con...
> webrtc/modules/congestion_controller/transport_feedback_adapter.h:76:
> rtc::ThreadChecker last_packet_feedback_vector_thread_checker_;
> On 2017/02/23 14:45:51, minyue-webrtc wrote:
> > one cannot bind a thread checker to a data.  Rather, it is to check whether
a 
> > method is called on a correct thread. therefore, it is a bit awkward to call
> > this last_packet_feedback_vector_thread_checker_.
> > 
> > So I think it would be better to call this feedback_access_thread_checker_
or
> > similar.
> > 
> > But I like a lock even more.
> > 
> 
> 1. I'm introducing checkers now to all of the functions, as per the last
> comment. They'll be named after their threads' function, not after the data
they
> protect.
> 2. Stefan preferred a thread checker. Either decision seems okay to me in
> principle, but I prefer the checker, since I see no reason to incur the cost
of
> a lock when we only ever touch this from one thread.

naming after the functionality of the threads will be good. but current name
"last_packet_feedback_vector_thread_checker_" is a bit too narrow too me.

[elad.alon_webrtc.org, 2017-02-23 15:13:23]

On 2017/02/23 15:05:18, minyue-webrtc wrote:
> On 2017/02/23 14:52:01, elad.alon wrote:
> >
>
https://codereview.webrtc.org/2710213003/diff/1/webrtc/modules/congestion_con...
> > File webrtc/modules/congestion_controller/transport_feedback_adapter.h
> (right):
> > 
> >
>
https://codereview.webrtc.org/2710213003/diff/1/webrtc/modules/congestion_con...
> > webrtc/modules/congestion_controller/transport_feedback_adapter.h:76:
> > rtc::ThreadChecker last_packet_feedback_vector_thread_checker_;
> > On 2017/02/23 14:45:51, minyue-webrtc wrote:
> > > one cannot bind a thread checker to a data.  Rather, it is to check
whether
> a 
> > > method is called on a correct thread. therefore, it is a bit awkward to
call
> > > this last_packet_feedback_vector_thread_checker_.
> > > 
> > > So I think it would be better to call this feedback_access_thread_checker_
> or
> > > similar.
> > > 
> > > But I like a lock even more.
> > > 
> > 
> > 1. I'm introducing checkers now to all of the functions, as per the last
> > comment. They'll be named after their threads' function, not after the data
> they
> > protect.
> > 2. Stefan preferred a thread checker. Either decision seems okay to me in
> > principle, but I prefer the checker, since I see no reason to incur the cost
> of
> > a lock when we only ever touch this from one thread.
> 
> naming after the functionality of the threads will be good. but current name
> "last_packet_feedback_vector_thread_checker_" is a bit too narrow too me.

I meant to say that "last_packet_feedback_vector_thread_checker_" is definitely
going away, to be replaced by a number of checkers with names of the type
"packetizer_thread_checker_", "socket_thread_checker_" and similar.

[elad.alon_webrtc.org, 2017-02-23 15:42:14]

Description was changed from

==========
Add thread-checking around TransportFeedbackAdapter::
last_packet_feedback_vector_ access

BUG=
==========

to

==========
Add thread-checking to TransportFeedbackAdapter.

BUG=
==========

[stefan-webrtc, 2017-02-24 14:48:54]

looks good, but maybe we should remove one of them?

https://codereview.webrtc.org/2710213003/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right):

https://codereview.webrtc.org/2710213003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/transport_feedback_adapter.cc:75:
RTC_DCHECK(packetizer_thread_checker_.CalledOnValidThread());
Maybe the value of this is small? WDYT?

[elad.alon_webrtc.org, 2017-02-27 10:55:30]

https://codereview.webrtc.org/2710213003/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right):

https://codereview.webrtc.org/2710213003/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/transport_feedback_adapter.cc:75:
RTC_DCHECK(packetizer_thread_checker_.CalledOnValidThread());
On 2017/02/24 14:48:53, stefan-webrtc wrote:
> Maybe the value of this is small? WDYT?

Hard for me to say. It does make sure no one ever tries to call this function
from two different threads, but that does seem unlikely. Could be argued it
would be nice to have this here for symmetry's sake. Also, we could check this
isn't called on the other threads (make sure the threads are distinct, which we
don't do now). Or remove completely. Which do you prefer?

[michaelt, 2017-02-27 11:29:50]

What is the value of adding ThreadChecker's, if we have already locks to protect
the data ?

[elad.alon_webrtc.org, 2017-02-27 11:36:10]

On 2017/02/27 11:29:50, michaelt wrote:
> What is the value of adding ThreadChecker's, if we have already locks to
protect
> the data ?

1. last_packet_feedback_vector_ is not protected by a lock, because it's only
ever accessed from one thread. The thread-checker makes sure of it.
2. The other checkers make it clearer which threads are involved, while making
sure that the documentation they provide remains correct.
3. In general, locks do not always remove the necessity for a thread-checker. It
can be that a lock is introduced to synchronize threads A and B, and someone
goes and adds thread C, which is still synchronized, but which doesn't fit the
design. The thread-checker would catch that.

[michaelt, 2017-02-27 13:13:09]

1. Ok, then we can do ThreadChecker for OnTransportFeedback and
GetTransportFeedbackVector.
2. I'm not sure we should do this. Since we restrain the functions to use a
concert thread without a need for it. If i think this to the end, I end up
adding ThreadChecker to every function in the project to document which function
uses which thread.

[stefan-webrtc, 2017-02-27 13:24:38]

Since this is a fairly complicated class with many threads involved I think this
makes it easier to understand the design assumptions.

lgtm

[elad.alon_webrtc.org, 2017-03-03 13:41:20]

Description was changed from

==========
Add thread-checking to TransportFeedbackAdapter.

BUG=
==========

to

==========
Add thread-checking to TransportFeedbackAdapter.

BUG=None
==========

[elad.alon_webrtc.org, 2017-03-03 13:41:29]

The CQ bit was checked by elad.alon@webrtc.org

[commit-bot: I haz the power, 2017-03-03 13:41:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-03 14:00:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-03 14:00:20]

Try jobs failed on following builders:
  linux_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/23828)

[elad.alon_webrtc.org, 2017-03-03 18:20:32]

On 2017/03/03 14:00:20, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_rel on master.tryserver.webrtc (JOB_FAILED,
> http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/23828)

Having spoken to Stefan about the problems this introduces for unit-tests, we've
decided it's not worth the effort, and we'll scrap it.