Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2098753002:
[Cronet] Fix race with NetLogWithNetworkChangeEvents registration with NCN (Closed)
|
Created:
4 years, 6 months ago by pauljensen Modified:
4 years, 5 months ago Reviewers:
xunjieli CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Cronet] Fix race with NetLogWithNetworkChangeEvents registration with NCN
The NetLogWithNetworkChangeEvents needs to wait to register with the
NetworkChangeNotifier until it is created, otherwise registration silently
fails and the NetLog does not contain network change events.
BUG=623037
R=xunjieli
Committed: https://crrev.com/03226f05445777d5b15f9a33f47cf43abb8d06c3
Cr-Commit-Position: refs/heads/master@{#402155}
Patch Set 1 #Patch Set 2 : _ #
Total comments: 6
Patch Set 3 : extend comment #Messages
Total messages: 21 (7 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
a
Patchset #3 (id:80001) has been deleted
Patchset #2 (id:60001) has been deleted
Helen, PTAL.
I don't think this will get rid of the race. |g_net_log| is lazily initialized. The g_net_log.Get() calls in |InitializeOnNetworkThread| will be guaranteed to run after CronetLibraryLoader.ensureInitializedOnMainThread has completed on the UI thread. The g_net_log.Get() call that is racy is in StartNetLogToFile, which can be called from any Java thread.
On 2016/06/24 13:18:34, xunjieli wrote: > I don't think this will get rid of the race. > > |g_net_log| is lazily initialized. The g_net_log.Get() calls in > |InitializeOnNetworkThread| will be guaranteed to run after > CronetLibraryLoader.ensureInitializedOnMainThread has completed on the UI > thread. > The g_net_log.Get() call that is racy is in StartNetLogToFile, which can be > called from any Java thread. That race is unavoidable but inconsequential. The race that this CL solves is registering with the NCN *after* the NCN is created, rather than racing NCN creation and NCN registration. As long as this logger is registered with the NCN early on, it should be fine. There won't be any NCN events to log until there is an NCN, so we won't really be missing anything. The rest of the netlog events come from the network thread, which hasn't spun up until after CronetURLRequestContextAdapter::InitRequestContextOnMainThread() posts CronetURLRequestContextAdapter::InitializeOnNetworkThread(), so we won't miss any of those either.
Talked to Paul offline, and understood the change better. https://codereview.chromium.org/2098753002/diff/100001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2098753002/diff/100001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:426: g_net_log.Get().EnsureInitializedOnMainThread(); Why do we initialize the observer on the main thread? It seems that doing it in InitializeOnNetworkThread() also achieves the same purpose. It is might better since the observer callbacks will be called on the network thread so we do fewer things on the UI thread?
https://codereview.chromium.org/2098753002/diff/100001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2098753002/diff/100001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:426: g_net_log.Get().EnsureInitializedOnMainThread(); On 2016/06/24 13:49:53, xunjieli wrote: > Why do we initialize the observer on the main thread? > It seems that doing it in InitializeOnNetworkThread() also achieves the same > purpose. It is might better since the observer callbacks will be called on the > network thread so we do fewer things on the UI thread? A few reasons: 1. so as to initialize it ASAP so we don't miss any events 2. so I don't have to make NetLogWithNetworkChangeEvents::EnsureInitializedOnMainThread thread-safe because it's called on only one thread as it stands now. This better matches Chrome's threading model which discourages thread-safe code: http://dev.chromium.org/developers/design-documents/threading 3. network threads can come and go, but the NetLog lives forever, so I need the logger to receive callbacks on a thread that lives forever, otherwise we'll crash when the network thread goes away and we try and post to it from the NCN 4. when communicating with the NCN I prefer to use the UI thread as this is what thread the NCN runs on, so I can possibly avoid some locking and races by calling the NCN on the UI thread.
https://codereview.chromium.org/2098753002/diff/100001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2098753002/diff/100001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:426: g_net_log.Get().EnsureInitializedOnMainThread(); On 2016/06/24 14:09:43, pauljensen wrote: > On 2016/06/24 13:49:53, xunjieli wrote: > > Why do we initialize the observer on the main thread? > > It seems that doing it in InitializeOnNetworkThread() also achieves the same > > purpose. It is might better since the observer callbacks will be called on the > > network thread so we do fewer things on the UI thread? > > A few reasons: > 1. so as to initialize it ASAP so we don't miss any events > 2. so I don't have to make > NetLogWithNetworkChangeEvents::EnsureInitializedOnMainThread thread-safe because > it's called on only one thread as it stands now. This better matches Chrome's > threading model which discourages thread-safe code: > http://dev.chromium.org/developers/design-documents/threading AddObserver is already thread safe. There isn't additional thing that is needed to ensure thread safety. > 3. network threads can come and go, but the NetLog lives forever, so I need the > logger to receive callbacks on a thread that lives forever, otherwise we'll > crash when the network thread goes away and we try and post to it from the NCN That is fair, although we could remove observer when the network thread is torn down. > 4. when communicating with the NCN I prefer to use the UI thread as this is what > thread the NCN runs on, so I can possibly avoid some locking and races by > calling the NCN on the UI thread. io_thread.cc initializes the logging observer on the browser i/o thread instead of on the UI thread. Maybe worth a comment here and/or in logging_network_change_observer.h on which thread the observer should be created on. When calling into NetLog, we will also try to grab a lock which is often held by the network thread (https://cs.chromium.org/chromium/src/net/log/net_log.cc?rcl=0&l=410). Acquiring lock on the UI thread isn't a whole lot better than posting a task to the network thread to add the event.
https://codereview.chromium.org/2098753002/diff/100001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2098753002/diff/100001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:426: g_net_log.Get().EnsureInitializedOnMainThread(); On 2016/06/24 16:20:11, xunjieli wrote: > On 2016/06/24 14:09:43, pauljensen wrote: > > On 2016/06/24 13:49:53, xunjieli wrote: > > > Why do we initialize the observer on the main thread? > > > It seems that doing it in InitializeOnNetworkThread() also achieves the same > > > purpose. It is might better since the observer callbacks will be called on > the > > > network thread so we do fewer things on the UI thread? > > > > A few reasons: > > 1. so as to initialize it ASAP so we don't miss any events > > 2. so I don't have to make > > NetLogWithNetworkChangeEvents::EnsureInitializedOnMainThread thread-safe > because > > it's called on only one thread as it stands now. This better matches Chrome's > > threading model which discourages thread-safe code: > > http://dev.chromium.org/developers/design-documents/threading > > AddObserver is already thread safe. There isn't additional thing that is needed > to ensure thread safety. No, take a look at EnsureInitializedOnMainThread(), access to net_change_logger_ is not atomic. Writing thread-safe functions is tricky and fraught with peril; this is why the Chromium threading policy recommends against thread-safe code. > > > 3. network threads can come and go, but the NetLog lives forever, so I need > the > > logger to receive callbacks on a thread that lives forever, otherwise we'll > > crash when the network thread goes away and we try and post to it from the NCN > > That is fair, although we could remove observer when the network thread is torn > down. It's possible, but it would require the addition of thread-safe reference counting which would be fairly ugly. > > > 4. when communicating with the NCN I prefer to use the UI thread as this is > what > > thread the NCN runs on, so I can possibly avoid some locking and races by > > calling the NCN on the UI thread. > > io_thread.cc initializes the logging observer on the browser i/o thread instead > of on the UI thread. Maybe worth a comment here and/or in > logging_network_change_observer.h on which thread the observer should be created > on. Are there restrictions about which thread the observer can be created on? AFAIK it can be created on any thread. > > When calling into NetLog, we will also try to grab a lock which is often held by > the network thread > (https://cs.chromium.org/chromium/src/net/log/net_log.cc?rcl=0&l=410). Acquiring > lock on the UI thread isn't a whole lot better than posting a task to the > network thread to add the event. Posting tasks and taking tasks off the queue requires locking anyhow. The operations performed by the LoggingNetworkChangeObserver are so tiny and they happen very very rarely (once every few minutes at best) so I don't think it's worth trying to optimize given I don't see an easy way to do so without involving thread-safe coding given the multi-threaded nature of the NetLog and NCN.
lgtm. thanks for the explanations! https://codereview.chromium.org/2098753002/diff/100001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2098753002/diff/100001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:72: // called on the UI thread as it is not thread-safe and the UI thread is I think I understand it slightly better now. Thanks for the explanations! Could you add a comment here to mention that it is not thread-safe to access |net_change_logger_| because we might have multiple CronetEngine and hence multiple network threads?
https://codereview.chromium.org/2098753002/diff/100001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2098753002/diff/100001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:72: // called on the UI thread as it is not thread-safe and the UI thread is On 2016/06/24 19:13:03, xunjieli wrote: > I think I understand it slightly better now. Thanks for the explanations! > Could you add a comment here to mention that it is not thread-safe to access > |net_change_logger_| because we might have multiple CronetEngine and hence > multiple network threads? Done.
The CQ bit was checked by pauljensen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/2098753002/#ps120001 (title: "extend comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Fix race with NetLogWithNetworkChangeEvents registration with NCN The NetLogWithNetworkChangeEvents needs to wait to register with the NetworkChangeNotifier until it is created, otherwise registration silently fails and the NetLog does not contain network change events. BUG=623037 R=xunjieli ========== to ========== [Cronet] Fix race with NetLogWithNetworkChangeEvents registration with NCN The NetLogWithNetworkChangeEvents needs to wait to register with the NetworkChangeNotifier until it is created, otherwise registration silently fails and the NetLog does not contain network change events. BUG=623037 R=xunjieli Committed: https://crrev.com/03226f05445777d5b15f9a33f47cf43abb8d06c3 Cr-Commit-Position: refs/heads/master@{#402155} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/03226f05445777d5b15f9a33f47cf43abb8d06c3 Cr-Commit-Position: refs/heads/master@{#402155} |
