Issue 29332660: Issue #3391 - Add detached initialization class
DescriptionIssue #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 #
MessagesTotal messages: 12
This is the first of a series of change sets to clean up detached initialization. This one consists of just the general code that performs all the control and synchronization.
First of all I would like to know the opinion of Oleksandr regarding the proposed approach. For me it does not look good, I would like to have something like concurrency::task (from Microsoft PPL) or use std::future & std::promise & std::thread (but they are broken in Visual Studio 2012 and 2013, I have not tested yet for 2015. I can only confirm that they were expectedly working in boost). I don't like such inheriting because I find it quite error-prone and non-transparency that some class has asynchronous initialization and the caller should firstly explicitly wait for it. However, JIC, I have made a portion of comments, some of them are in this codereview and some are in https://codereview.adblockplus.org/29334397/, I would like to have it in one review. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... File src/plugin/DetachedInitialization.h (right): https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:28: * A detached initializer for static class members It would be much better to pull the changes of this file from the following review into this review https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/Detached... https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:93: typedef std::unique_lock<std::mutex> UniqueLockType; I think we don't need "Type" suffix. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:94: typedef std::lock_guard<std::mutex> LockGuardType; UniqueLockType is used only once, LockGuardType is used twice, why do we need a typedef for them? https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:96: inline bool AlreadyInitialized() // noexcept It seems this method should be `const`. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:99: return isInitializationComplete.load(std::memory_order_acquire); Do we really need to be so specific and not use a default memory order value everywhere in this class? It rather attracts the attention and causes questions than simplifies the understanding (readability). Thus we don't need these methods. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:102: inline bool AlreadyStartedInitializer() // noexcept It seems this method should be `const`. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:116: catch (std::exception& e) Compiler generates warning `e` is not used. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:116: catch (std::exception& e) Why is it not const reference? https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:118: // Assert initializer threw an exception This comment is not needed https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:125: LockGuardType lg(mutex); it would be easier to read if name it `lock` not `lg`. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:141: UniqueLockType ul(mutex); // won't throw because this class uses only scoped locks it would be easier to read if name it `lock` not `ul`. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:153: // Should not reach It would be better to put `assert(false && "Should not reach")` here. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:173: * By precondition at most the initializer thread is running, and it doesn't touch this flag. We don't need this comment. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:181: } We should wait in the destructor for the finishing of the thread, otherwise we can get the case when the object instance has been already destroyed but the variables from the scope of ThreadMain are not destroyed yet but they use in their destructor object instance members (in particular `lg` uses `mutex`). https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:188: * - if throws, AlreadyStartedInitializer is false Why is it false and what happens in EnsureInitialized called from the destructor then? https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:207: LockGuardType lg(mutex, std::adopt_lock); Why not just use `std::lock_guard<std::mutex> lock(mutex)` without try_lock and the infinite cycle with sleep? https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:220: std::thread th([this]() -> void { ThreadMain(); }); Can it be written down as `[this]{ ThreadMain(); }`? https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:223: th.detach(); // Won't throw because the thread is both valid and joinable We should not do that, see comment for the destructor. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:234: std::this_thread::sleep_for(std::chrono::microseconds(1)); std::this_thread::yield() would be better. FYI: according to my experience the precision of Sleep on MS Windows (NT) is about 20 msec (milli not micro), it also means that usually it could not sleep less than ~20msec, so it would be better to remove the comment. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:248: void EnsureInitialized() It would be better to name it WaitForInitCompletion. "EnsureInitialized" implies that SpawnInitializer() will be called if it's not called yet. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:248: void EnsureInitialized() If you require to call firstly SpawnInitializer() then add an `assert` or throw an exception.
On 2016/01/29 10:04:24, sergei wrote: > First of all I would like to know the opinion of Oleksandr regarding the > proposed approach. And I would like to know your opinion. If you can identify defects in the present code, I'd like to hear them. If you cannot identify defects in this code, then you shouldn't stand in the way of making progress. If you don't understand the code, say so. I consider all the code I write for ABP under-documented, but that's the ABP policy. > For me it does not look good, I would like to have something > like concurrency::task (from Microsoft PPL) or use std::future & std::promise & > std::thread You do realize that none of those have atomic, run-once semantics, right? They are not replacements for the present code. And, I do hope you noticed, the present code _does_ use std::thread. > I don't like such inheriting because I find it quite error-prone and > non-transparency that some class has asynchronous initialization and the caller > should firstly explicitly wait for it. _Everything_ involving asynchronous initialization is error-prone. The whole point of this detached initialization class is that you can isolate all the possible errors that can't be avoided into the class interface and hide the rest of them inside the class implementation, ready for reuse. One of the unavoidable properties is that you have to wait for initialization to complete before you use the class. Can't get around that one in any way. Any class that has detached initialization needs to document that somehow. There are two basic approaches: 1) Expose an initialization-complete interface so that the caller can wait exactly once and then use the object ordinarily afterwards. 2) Use EnsureInitialized() internally and document the methods that can block until initialization is complete. Both approach are valid depending on circumstances. The present class supports both approaches and it does not prefer one over the other. That's how it should be. > However, JIC, I have made a portion of comments, some of them are in this > codereview and some are in https://codereview.adblockplus.org/29334397/, I would > like to have it in one review. That ship has sailed already. I am unwilling to make a wholesale restructuring to my repository to accommodate the fact that you did not make a timely review in the first place. The second batch of modifications, not incidentally, is rather small and does not touch any of the multiprocessing issues. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... File src/plugin/DetachedInitialization.h (right): https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:28: * A detached initializer for static class members On 2016/01/29 10:04:21, sergei wrote: > It would be much better to pull the changes of this file from the following > review into this review > https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/Detached... No need. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:93: typedef std::unique_lock<std::mutex> UniqueLockType; On 2016/01/29 10:04:22, sergei wrote: > I think we don't need "Type" suffix. It's the C++ library convention, modified for our capitalization style. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:94: typedef std::lock_guard<std::mutex> LockGuardType; On 2016/01/29 10:04:22, sergei wrote: > UniqueLockType is used only once, LockGuardType is used twice, why do we need a > typedef for them? They're present because the code at the point of use is clearer with them. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:96: inline bool AlreadyInitialized() // noexcept On 2016/01/29 10:04:22, sergei wrote: > It seems this method should be `const`. Done. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:99: return isInitializationComplete.load(std::memory_order_acquire); On 2016/01/29 10:04:22, sergei wrote: > Do we really need to be so specific and not use a default memory order value > everywhere in this class? Memory fences are absolutely critical for correct behavior. I'm not going to try to teach you _why_ that is. If you don't understand why they're necessary, then start with the two URL's in the class documentation comment above, and keep reading. Keep reading until you understand why they're here. > It rather attracts the attention and causes questions > than simplifies the understanding (readability). It _should_ attract attention. The code is incorrect without these fences. > Thus we don't need these methods. Here's how I read this statement: "I don't understand it, therefore it shouldn't be here." I do hope you appreciate why I have zero respect for this statement. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:102: inline bool AlreadyStartedInitializer() // noexcept On 2016/01/29 10:04:20, sergei wrote: > It seems this method should be `const`. Done. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:116: catch (std::exception& e) On 2016/01/29 10:04:21, sergei wrote: > Compiler generates warning `e` is not used. Done. It's fixed in the subsequent patch set, incidentally, but since I need to post a new patch set, I can do it here. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:125: LockGuardType lg(mutex); On 2016/01/29 10:04:20, sergei wrote: > it would be easier to read if name it `lock` not `lg`. Its name doesn't matter, since it's a sentry object and only instantiated. Might as well call it 'x'. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:153: // Should not reach On 2016/01/29 10:04:21, sergei wrote: > It would be better to put `assert(false && "Should not reach")` here. That would be unnecessary code bloat. There's a proof in the code comments about why the code will not throw. It relies upon guarantees made by the standard library. The try-block is only there so we can get a noexcept declaration to compile correctly. If C++ were more sophisticated, we could show noexcept behavior inside non-comment code. But we can't. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:173: * By precondition at most the initializer thread is running, and it doesn't touch this flag. On 2016/01/29 10:04:23, sergei wrote: > We don't need this comment. We do. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:181: } On 2016/01/29 10:04:21, sergei wrote: > We should wait in the destructor for the finishing of the thread Sergei, we _are_ waiting for the destructor to finish. That's what calling EnsureInitialized() does. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:188: * - if throws, AlreadyStartedInitializer is false On 2016/01/29 10:04:21, sergei wrote: > Why is it false and what happens in EnsureInitialized called from the destructor > then? Trace the code. The only times this function can throw is when 'AlreadyStartedInitializer' is false. Analyzing this isn't as simple as identifying statements that _might_ throw. Most such statements cannot actually throw because their preconditions for not throwing are satisfied. For the record, the only statement that can throw is the thread constructor. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:207: LockGuardType lg(mutex, std::adopt_lock); On 2016/01/29 10:04:22, sergei wrote: > Why not just use `std::lock_guard<std::mutex> lock(mutex)` without try_lock and > the infinite cycle with sleep? Because that would be incorrect code. There's a race condition if you do it in the way you suggest. I know, because that's what I had originally. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:223: th.detach(); // Won't throw because the thread is both valid and joinable On 2016/01/29 10:04:22, sergei wrote: > We should not do that, see comment for the destructor. Calling 'detach()' here is the whole point of detached initialization. What's not clear here? If we don't call 'detach()' here, we get a completely different class. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:234: std::this_thread::sleep_for(std::chrono::microseconds(1)); On 2016/01/29 10:04:23, sergei wrote: > std::this_thread::yield() would be better. Calling 'sleep()' is more conservative. This statement only gets executed in the case of a spurious 'try_lock()' failure. That's already rare on any architecture. I read some commentary online that the VS version of this never has spurious failures. The upshot is that this statement is executed seldom-to-never. Hence the performance of this statement is not critical. I've picked the statement that has more conservative behavior. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:248: void EnsureInitialized() On 2016/01/29 10:04:21, sergei wrote: > If you require to call firstly SpawnInitializer() then add an `assert` or throw > an exception. As I've said so many times before, this is a not a general purpose library class. We do not need to hold the hands of potential users. We have two uses of this class in the code base. I'm sure we can get it right. If this requirement were undocumented, we would have a problem. But it's adequately documented. Therefore, all I can say is "please don't shoot yourself in the foot by not reading". https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:248: void EnsureInitialized() On 2016/01/29 10:04:21, sergei wrote: > It would be better to name it WaitForInitCompletion. "EnsureInitialized" implies > that SpawnInitializer() will be called if it's not called yet. Renaming suggestion rejected. The current name reflects what you can rely on ("what it does"). The name you suggest is how it works.
Patch set 2 adds two 'const' declarations and gets rid of a compiler warning error. It also includes a change in the gyp file from a rebase.
I am not sure if we need this class to begin with. I think we can do detached initialization in the two places where this is going to be used just as well. What are the arguments to move this functionality into the separate class? As it is now it definitely is hard to understand, and there's surely too much text in comments. I have also suggested another implementation, please let me know what you think about that. https://codereview.adblockplus.org/29332660/diff/29335109/src/plugin/Detached... File src/plugin/DetachedInitialization.h (right): https://codereview.adblockplus.org/29332660/diff/29335109/src/plugin/Detached... src/plugin/DetachedInitialization.h:190: void SpawnInitializer() The double checked locking is really convoluted here. Why not just do something like? std::once_flag flag; ThreadMain() { ... std::call_once(flag, F); cv.notify_all(); } void SpawnInitializer() { ... std::thread th([this]() -> void { ThreadMain(); }); th.detach(); } And I would also still use the
On 2016/02/01 02:47:10, Oleksandr wrote: > I am not sure if we need this class to begin with. I think we can do detached > initialization in the two places where this is going to be used just as well. > What are the arguments to move this functionality into the separate class? Exactly as stated in the description for #3391. The current code is defective, in part because even though detached initialization is conceptually simple, its implementation is fairly subtle and the current implementation fails in certain ways. Putting it in a separate class allows it to be tested separately. If you don't have a separate class, you can't get separate unit tests for this piece of behavior. And that makes it more difficult to get correct code. > As it is now it definitely is hard to understand, and there's surely too much > text in comments. In the present case the issue is fundamentally hard to understand. Having significantly fewer comments would make it even harder to understand. There may be a few extraneous comments; we can go over those once we decide the non-comment code is acceptable. > I have also suggested another implementation, please let me > know what you think about that. It doesn't work as well. Its performance is worse. It's also not as simple as presented, since you have to work around the behavior of 'call_once()' with more code. https://codereview.adblockplus.org/29332660/diff/29335109/src/plugin/Detached... File src/plugin/DetachedInitialization.h (right): https://codereview.adblockplus.org/29332660/diff/29335109/src/plugin/Detached... src/plugin/DetachedInitialization.h:190: void SpawnInitializer() On 2016/02/01 02:47:09, Oleksandr wrote: > std::once_flag flag; [...] > std::call_once(flag, F); 'std::call_once' has different behavior than might be obvious, and it leads to behavior that does not work as well here. The main problem is that using 'call_once()' has exception behavior that's inappropriate for what we need. If you spawn multiple threads and the active execution thread throws, this function then converts one of the passive threads into the active execution thread and tries again. In other words, the function should not be named 'call_once()' but 'call_until_first_return()'. There are uses where that's appropriate behavior, but this isn't one of them. Another problem is that using 'call_once()' you have to create one thread per call to 'SpawnInitializer()'. The present approach only ever creates a single thread over all instances.
https://codereview.adblockplus.org/29332660/diff/29335109/src/plugin/Detached... File src/plugin/DetachedInitialization.h (right): https://codereview.adblockplus.org/29332660/diff/29335109/src/plugin/Detached... src/plugin/DetachedInitialization.h:3: * Copyright (C) 2006-2015 Eyeo GmbH Nit: 2016 https://codereview.adblockplus.org/29332660/diff/29335109/src/plugin/Detached... src/plugin/DetachedInitialization.h:190: void SpawnInitializer() On 2016/02/03 14:19:01, Eric wrote: > On 2016/02/01 02:47:09, Oleksandr wrote: > The main problem is that using 'call_once()' has exception behavior that's > inappropriate for what we need. If you spawn multiple threads and the active > execution thread throws, this function then converts one of the passive threads > into the active execution thread and tries again. In other words, the function > should not be named 'call_once()' but 'call_until_first_return()'. There are > uses where that's appropriate behavior, but this isn't one of them. I am not convinced converting passive threads into active ones isn't what we want though. As it is now, re-throwing the exception in EnsureInitialized is counter intuitive, IMHO. In fact I think we could be better off if we eat the possible exceptions in the initialization function instead and assume it can not throw. > > Another problem is that using 'call_once()' you have to create one thread per > call to 'SpawnInitializer()'. The present approach only ever creates a single > thread over all instances. With the usage that this class will have I don't think it will be any performance hit at all. On the flip side would be a better readability/maintainability though.
Patch set 3 updates the copyright year. https://codereview.adblockplus.org/29332660/diff/29335109/src/plugin/Detached... File src/plugin/DetachedInitialization.h (right): https://codereview.adblockplus.org/29332660/diff/29335109/src/plugin/Detached... src/plugin/DetachedInitialization.h:3: * Copyright (C) 2006-2015 Eyeo GmbH On 2016/02/05 07:36:05, Oleksandr wrote: > Nit: 2016 Done. https://codereview.adblockplus.org/29332660/diff/29335109/src/plugin/Detached... src/plugin/DetachedInitialization.h:190: void SpawnInitializer() On 2016/02/05 07:36:05, Oleksandr wrote: > I am not convinced converting passive threads into active ones isn't what we > want though. It's not what we want, at least not now. The difference in semantics is that between handling hard errors and soft errors. If you have hard errors, you don't want to retry. And if your apparatus does retry, you may need new code in the initialization function not to actually retry, since some hard errors don't like being tried again. On the other hand, if you have soft errors, you may want to retry, but you need to be able to distinguish that the error is hard vs. soft and not combine the two. We have no capacity to make that distinction yet. We haven't even done a proper analysis of what error might be soft vs. hard, although the one I've looked that replace static initialization look all/mostly like hard errors. In addition, when you want to handle soft errors, you need to have class semantics to allow an object to exist in a "temporarily not working" state. We don't have any capacity at present for that either. The semantics of this present class are for hard errors. This makes everything easier and we can make incremental progress. If we want to support soft errors at the outset, it's a lot more code change. https://codereview.adblockplus.org/29332660/diff/29335109/src/plugin/Detached... src/plugin/DetachedInitialization.h:190: void SpawnInitializer() On 2016/02/05 07:36:05, Oleksandr wrote: > As it is now, re-throwing the exception in EnsureInitialized is > counter intuitive, IMHO. There's no other place to throw it. You can't throw it from 'SpawnInitializer()' since that function returns before the initializer thread even starts. What's going on here is something like a deferred constructor exception. When 'EnsureInitialized()' throws, it's telling you that it's not initialized and is never going to be. > In fact I think we could be better off if we eat the > possible exceptions in the initialization function instead and assume it can not > throw. You can never assume that a function call will not throw (even if declared 'noexcept'), because there are a handful of exceptions, such as 'bad_alloc', that are implicit in the language. Besides, you can get what you want in the present code just fine. Just use an initializer function that doesn't throw. That's exactly what I did in https://codereview.adblockplus.org/29332665/ (which, I must point out, has _still_ received no comments). > With the usage that this class will have I don't think it will be any > performance hit at all. Threads are reasonably heavy system resources. (Not as much as a process, but still ...) It's foolish code that spawns a thread just to check some flags and then terminates, particularly when we've already got code (the present review) that does that completely generically already. > On the flip side would be a better > readability/maintainability though. Because the semantics are different (hard vs. soft errors), this concern isn't relevant. We need it to behave correctly.
On 2016/02/08 17:34:07, Eric wrote: > You can never assume that a function call will not throw (even if declared > 'noexcept'), because there are a handful of exceptions, such as 'bad_alloc', > that are implicit in the language. My point was that we can move try-catch block into the initialization function F itself. It is our function after all. That is why we can assume that it will not throw - we would just have a requirement for it not to. And even if at some point it does throw, assuming it is a "soft error" is just as good as assuming it is a "hard error", I think. Like you said we have no way of knowing what went wrong, really. Anyway, it seems we simply disagree here on a conceptual issue. Let's have Sergei weigh in here, I think. We can move further when we approve/disapprove the call_once method. @Sergei, please comment.
On 2016/02/09 05:03:22, Oleksandr wrote: > My point was that we can move try-catch block into the initialization function F > itself. It is our function after all. That is why we can assume that it will not > throw - we would just have a requirement for it not to. Thread-main functions are entry points. Each needs an exception handler to catch the (rare) cases where the runtime throws, even when nothing in your code does explicitly. Above them in the call stack is non-C++ code, where the stack unwinding code will fail. You can't do without a 'try' statement here. So at the very least you can't "move" the 'try' statement. As I said before, there is already an exception handler around the initialization function. It is there for *different* purposes. (1) To move the time at which we first see the error into the initializer function (and not, say, in 'EnsureInitialized()' or whatever other mechanism might possibly exist to get at an exception later). (2) To avoid suppressing errors, which is simply bad practice, which is what would happen if you simply ignored an exception everywhere. You've said before you don't like exception handlers in the code. Well, they're part of C++, and high-quality code needs to deal with exception safety, however inconvenient. > And even if at some > point it does throw, assuming it is a "soft error" is just as good as assuming > it is a "hard error", I think. You need extra infrastructure to handle soft errors, which we don't have in place. It is safer to assume, for now, that we have hard errors. I suspect you haven't even looked at https://codereview.adblockplus.org/29332665/, which is the first review that uses static initialization. I only moved part of the static initialization in that change set in order to clarify the change. The code in the initializer loads DLL "uxtheme.dll" and extracts some entry points from it. If this fails, it's a hard error. There's really no point in trying again to load a system DLL that didn't load the first time. More importantly, we've got no code for graceful degradation in case such system resources are not present. I am not averse to dealing with soft errors. I am, however, completely averse to dealing with them badly, which would be the case if we simply made detached initialization treat everything as a soft error without also adding the code to deal with the semantics of soft errors. "The best is the enemy of the good". We need incremental progress on this issue. That's what's in the present review. > We can move further when we > approve/disapprove the call_once method. I cannot approve of 'call_once'. If you insist on it for pure formality sake, we'll need to write code to get rid of its implicit retry behavior. It's not a win.
On 2016/02/09 05:03:22, Oleksandr wrote: > Let's have Sergei weigh in here, I think. We can move further when we > approve/disapprove the call_once method. @Sergei, please comment. I personally think that creating many threads is not a good idea here, however it seems we can use std::flag_once for the method which spawns the thread, not for the worker function. Nevertheless, as I said above I disagree with the proposed approach, I would like to have something which rather wraps the result value. May be it makes more sense to come up with some code. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... File src/plugin/DetachedInitialization.h (right): https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:28: * A detached initializer for static class members On 2016/01/29 15:33:10, Eric wrote: > On 2016/01/29 10:04:21, sergei wrote: > > It would be much better to pull the changes of this file from the following > > review into this review > > > https://codereview.adblockplus.org/29334397/diff/29334398/src/plugin/Detached... > > No need. Currently it's very inconvenient to review the code in two places. Why do you want us to review and to commit a temporary implementation which is already different? It should be in one commit. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:93: typedef std::unique_lock<std::mutex> UniqueLockType; On 2016/01/29 15:33:10, Eric wrote: > On 2016/01/29 10:04:22, sergei wrote: > > I think we don't need "Type" suffix. > > It's the C++ library convention, modified for our capitalization style. Acknowledged. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:94: typedef std::lock_guard<std::mutex> LockGuardType; On 2016/01/29 15:33:11, Eric wrote: > On 2016/01/29 10:04:22, sergei wrote: > > UniqueLockType is used only once, LockGuardType is used twice, why do we need > a > > typedef for them? > > They're present because the code at the point of use is clearer with them. That's very subjective, OK, let's leave them. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:99: return isInitializationComplete.load(std::memory_order_acquire); On 2016/01/29 15:33:11, Eric wrote: > On 2016/01/29 10:04:22, sergei wrote: > > Do we really need to be so specific and not use a default memory order value > > everywhere in this class? > > Memory fences are absolutely critical for correct behavior. That's clear that they are critical, why do we need to use not a default value (`std::memory_order_seq_cst`)? > > I'm not going to try to teach you _why_ that is. If you don't understand why > they're necessary, then start with the two URL's in the class documentation > comment above, and keep reading. Keep reading until you understand why they're > here. That sounds quite offensive, taking into account comments from some other code reviews, I can only recommend you to behave professional in conversations, read the documentation and carefully check the everything before you write an answer and don't demonstrate the lack of experience. Try to read something about ethics in code review. > > > It rather attracts the attention and causes questions > > than simplifies the understanding (readability). > > It _should_ attract attention. The code is incorrect without these fences. I'm not proposing to not use these fences, they are still used with default value for memory order. There is a negligible overheard and much better maintainability. > > > Thus we don't need these methods. > > Here's how I read this statement: "I don't understand it, therefore it shouldn't > be here." I do hope you appreciate why I have zero respect for this statement. Why do think that I don't understand it? Just in case, when I ask the question "why" or "what" it also means that I'm kindly giving you an opportunity to recognize the trouble by yourself and don't want to be so rude as you above by saying "Keep reading until you understand". I think that your position in return to generate something, the code and not succinct lengthy answers in codereviews, is not productive, by doing it you are wasting at least my and Oleksandr's time, please respect the colleagues. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:116: catch (std::exception& e) On 2016/01/29 15:33:10, Eric wrote: > On 2016/01/29 10:04:21, sergei wrote: > > Compiler generates warning `e` is not used. > > Done. > > It's fixed in the subsequent patch set, incidentally, but since I need to post a > new patch set, I can do it here. I see that in https://codereview.adblockplus.org/29334397/diff/29335525/src/plugin/Detached... it's replaced by ellipsis, that's one of the examples that it would be better to pull the changes into the current codereview. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:118: // Assert initializer threw an exception On 2016/01/29 10:04:22, sergei wrote: > This comment is not needed ignored? https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:153: // Should not reach On 2016/01/29 15:33:11, Eric wrote: > On 2016/01/29 10:04:21, sergei wrote: > > It would be better to put `assert(false && "Should not reach")` here. > > That would be unnecessary code bloat. It's not more than the comment, however it helps to find it out when it happens. > > There's a proof in the code comments about why the code will not throw. It > relies upon guarantees made by the standard library. The try-block is only there > so we can get a noexcept declaration to compile correctly. > > If C++ were more sophisticated, we could show noexcept behavior inside > non-comment code. But we can't. > https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:181: } On 2016/01/29 15:33:10, Eric wrote: > On 2016/01/29 10:04:21, sergei wrote: > > We should wait in the destructor for the finishing of the thread > > Sergei, we _are_ waiting for the destructor to finish. That's what calling > EnsureInitialized() does. Right, overlooked. However, I would to still wait for the finishing of the thread for the sake of a good order because `this` is captured by the closure of thread function, it has no side-effect here but it attracts an attention. In addition, it's always safer to know when the thread has finished and we can do it here without a significant overhead. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:188: * - if throws, AlreadyStartedInitializer is false On 2016/01/29 15:33:11, Eric wrote: > On 2016/01/29 10:04:21, sergei wrote: > > Why is it false and what happens in EnsureInitialized called from the > destructor > > then? > > Trace the code. The only times this function can throw is when > 'AlreadyStartedInitializer' is false. Analyzing this isn't as simple as > identifying statements that _might_ throw. Most such statements cannot actually > throw because their preconditions for not throwing are satisfied. > > For the record, the only statement that can throw is the thread constructor. Acknowledged. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:207: LockGuardType lg(mutex, std::adopt_lock); On 2016/01/29 15:33:10, Eric wrote: > On 2016/01/29 10:04:22, sergei wrote: > > Why not just use `std::lock_guard<std::mutex> lock(mutex)` without try_lock > and > > the infinite cycle with sleep? > > Because that would be incorrect code. > > There's a race condition if you do it in the way you suggest. I know, because > that's what I had originally. What is the race condition here? If there is a real race condition then it should be reflected in the comment, right now the comment is not useful. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:223: th.detach(); // Won't throw because the thread is both valid and joinable On 2016/01/29 15:33:10, Eric wrote: > On 2016/01/29 10:04:22, sergei wrote: > > We should not do that, see comment for the destructor. > > Calling 'detach()' here is the whole point of detached initialization. What's > not clear here? If we don't call 'detach()' here, we get a completely different > class. If the thread is a member of the class then initialization won't block the caller. Detaching a thread usually results in some race conditions and less maintainable code. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:234: std::this_thread::sleep_for(std::chrono::microseconds(1)); On 2016/01/29 15:33:11, Eric wrote: > On 2016/01/29 10:04:23, sergei wrote: > > std::this_thread::yield() would be better. > > Calling 'sleep()' is more conservative. > > This statement only gets executed in the case of a spurious 'try_lock()' > failure. That's already rare on any architecture. I read some commentary online > that the VS version of this never has spurious failures. The upshot is that this > statement is executed seldom-to-never. Hence the performance of this statement > is not critical. I've picked the statement that has more conservative behavior. std::this_thread::yield() is more descriptive, so we need neither comments nor questionable sleep. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:248: void EnsureInitialized() On 2016/01/29 15:33:11, Eric wrote: > On 2016/01/29 10:04:21, sergei wrote: > > It would be better to name it WaitForInitCompletion. "EnsureInitialized" > implies > > that SpawnInitializer() will be called if it's not called yet. > > Renaming suggestion rejected. The current name reflects what you can rely on > ("what it does"). The name you suggest is how it works. It does not ensure that something is initialized if nobody called `SpawnInitializer`, so it does not do what is said in the name. https://codereview.adblockplus.org/29332660/diff/29332661/src/plugin/Detached... src/plugin/DetachedInitialization.h:248: void EnsureInitialized() On 2016/01/29 15:33:11, Eric wrote: > On 2016/01/29 10:04:21, sergei wrote: > > If you require to call firstly SpawnInitializer() then add an `assert` or > throw > > an exception. > > As I've said so many times before, this is a not a general purpose library > class. We do not need to hold the hands of potential users. > > We have two uses of this class in the code base. I'm sure we can get it right. If it's used merely two times then it might be not a time to generalize it. > > If this requirement were undocumented, we would have a problem. But it's > adequately documented. Therefore, all I can say is "please don't shoot yourself > in the foot by not reading". It's good that it's commented but having the check in addition in the code does not hurt.
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. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
