Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29332660: Issue #3391 - Add detached initialization class

Created:
Dec. 14, 2015, 6:06 p.m. by Eric
Modified:
Aug. 21, 2016, 5:05 p.m.
Reviewers:
sergei, Oleksandr
CC:
Felix Dahlke
Visibility:
Public.

Description

Issue #3391 - Add detached initialization class Add class DetachedInitializer(), which encapsulates the control of detached initialization in a separate thread. Add unit tests for this class. ----- Another review depends upon this one: https://codereview.adblockplus.org/29332665/

Patch Set 1 #

Total comments: 52

Patch Set 2 : address comments #

Total comments: 7

Patch Set 3 : fix copyright year #

Unified diffs Side-by-side diffs Delta from patch set Stats (+459 lines, -0 lines) Patch
M adblockplus.gyp View 1 2 chunks +3 lines, -0 lines 0 comments Download
A src/plugin/DetachedInitialization.h View 1 2 1 chunk +258 lines, -0 lines 0 comments Download
A test/plugin/DetachedInitializationTest.cpp View 1 2 1 chunk +198 lines, -0 lines 0 comments Download

Messages

Total messages: 12
Eric
This is the first of a series of change sets to clean up detached initialization. ...
Dec. 14, 2015, 6:15 p.m. (2015-12-14 18:15:20 UTC) #1
sergei
First of all I would like to know the opinion of Oleksandr regarding the proposed ...
Jan. 29, 2016, 10:04 a.m. (2016-01-29 10:04:24 UTC) #2
Eric
On 2016/01/29 10:04:24, sergei wrote: > First of all I would like to know the ...
Jan. 29, 2016, 3:33 p.m. (2016-01-29 15:33:09 UTC) #3
Eric
Patch set 2 adds two 'const' declarations and gets rid of a compiler warning error. ...
Jan. 29, 2016, 3:42 p.m. (2016-01-29 15:42:44 UTC) #4
Oleksandr
I am not sure if we need this class to begin with. I think we ...
Feb. 1, 2016, 2:47 a.m. (2016-02-01 02:47:10 UTC) #5
Eric
On 2016/02/01 02:47:10, Oleksandr wrote: > I am not sure if we need this class ...
Feb. 3, 2016, 2:19 p.m. (2016-02-03 14:19:00 UTC) #6
Oleksandr
https://codereview.adblockplus.org/29332660/diff/29335109/src/plugin/DetachedInitialization.h File src/plugin/DetachedInitialization.h (right): https://codereview.adblockplus.org/29332660/diff/29335109/src/plugin/DetachedInitialization.h#newcode3 src/plugin/DetachedInitialization.h:3: * Copyright (C) 2006-2015 Eyeo GmbH Nit: 2016 https://codereview.adblockplus.org/29332660/diff/29335109/src/plugin/DetachedInitialization.h#newcode190 ...
Feb. 5, 2016, 7:36 a.m. (2016-02-05 07:36:06 UTC) #7
Eric
Patch set 3 updates the copyright year. https://codereview.adblockplus.org/29332660/diff/29335109/src/plugin/DetachedInitialization.h File src/plugin/DetachedInitialization.h (right): https://codereview.adblockplus.org/29332660/diff/29335109/src/plugin/DetachedInitialization.h#newcode3 src/plugin/DetachedInitialization.h:3: * Copyright ...
Feb. 8, 2016, 5:34 p.m. (2016-02-08 17:34:07 UTC) #8
Oleksandr
On 2016/02/08 17:34:07, Eric wrote: > You can never assume that a function call will ...
Feb. 9, 2016, 5:03 a.m. (2016-02-09 05:03:22 UTC) #9
Eric
On 2016/02/09 05:03:22, Oleksandr wrote: > My point was that we can move try-catch block ...
Feb. 9, 2016, 2:29 p.m. (2016-02-09 14:29:11 UTC) #10
sergei
On 2016/02/09 05:03:22, Oleksandr wrote: > Let's have Sergei weigh in here, I think. We ...
Feb. 9, 2016, 5:38 p.m. (2016-02-09 17:38:16 UTC) #11
Eric
Aug. 21, 2016, 5:05 p.m. (2016-08-21 17:05:18 UTC) #12
Regarding `std::call_once`:

On 2016/02/05 07:36:06, Oleksandr wrote:
> I am not convinced converting passive threads into active ones isn't what we
> want though. 

Since this exchange first occurred, I've realized that we need to be able to
handle soft errors during detached initialization, because one of the underlying
activities can be network activity to fetch block lists. So the semantics in
this review of only handling errors as hard isn't the right thing.

On the other hand, I've verified that we can't use `std::call_once` because its
VS2012 implementation is terminally broken. This implementation (since improved)
uses a single global mutex to coordinate all `std::call_once` calls. This is
only a performance problem, not the terminally fatal one. The fatal problem is
that the implementation is also defective, for some reason, and looks like it
might involve a compiler defect. If the function argument of `call_once` throws,
then it fails to unlock the global mutex by skipping past its destructor
(verified by setting a breakpoint there). Since the mutex then can't be acquired
later, this variously manifests as hanging on subsequent calls, failure to
execute the function body at all, and errors at termination for closing while a
resource is busy.

Powered by Google App Engine
This is Rietveld