Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10909228:
Add PathService::RemoveOverride to clear path overrides. (Closed)
|
Created:
8 years, 3 months ago by pastarmovj Modified:
8 years, 2 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd PathService::RemoveOverride to clear path overrides.
This is especially useful for unit tests because they live in the same process
and share all the singletons including PathService. One can of course override
the overrides but it is much cleaner and sometimes the only good solution to
return to the original PathProvider instead.
BUG=149161
TEST=base_unittests:PathServiceTest.*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158842
Patch Set 1 #
Total comments: 15
Patch Set 2 : Refactored the locking semantics. #
Total comments: 4
Patch Set 3 : Addressed comments and a problem on windows. #Patch Set 4 : Cleaned up a typo in unit tests file. #
Total comments: 4
Patch Set 5 : Addressed suggestions. #Patch Set 6 : Made the tests even more OS independent. #
Total comments: 7
Patch Set 7 : Addressed nits. #
Messages
Total messages: 15 (0 generated)
Hi Jim, can you please review a small addition to the PathService which I intend on using to implement a ScopedDirUserDataOverride class to the unit test utils. It will be very helpful for unit tests that need to put and read files from the profile dir. Browser tests use similar mechanism to override the profile but they don't really care about cleaning it up because each test runs in its own process.
I wasn't fully convinced by the description or the bug. Why is this a good thing to do? I suspect it is a dangerous thing, even it test code, to mess with the overrides (and clear them). There is at least one race bug that was visible in the pre-existing code, and that probably should be fixed (different bug? cl?). If you're willing to do that, you should probably also do some of the cleanup as well. https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.cc File base/path_service.cc (right): https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.cc#n... base/path_service.cc:168: *result = it->second; This is (I think) the one place they *could* (should?) have put an override into the cache.... but it does not get done. It might be simpler to never use the cache for overridden values. To be cleaner, it is then probably nicer to check for the override while we have the lock and are checking for the cache, rather than acquiring the lock twice. This way we don't have to copy the override into a second map. https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.cc#n... base/path_service.cc:207: Provider* provider = path_data->providers; This is probably racy, as the contents of path_data->provider may arrive asynchronously, and worse yet, there are no acquire and release semantics. As a result, we might get the pointer intact... but what it points to might not be intact. The acquisition of the start of this list should be done under a lock, although it is true that we can iterate over the list without a lock. The corresponding lock is present at the end of the file (where we do the prepend). https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.cc#n... base/path_service.cc:224: // static I personally like this style... but it is a gratuitous style change to this file. You should just leave the existing style intact. https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.cc#n... base/path_service.cc:263: path_data->cache[key] = file_path; It is interesting that the directly implent into the cache when they establish the override. This is a little suspicious, but could have just been premature optimization. Note that the cache.clear() above would have undone other "premature optimization" done for other overrides. Given the odd performance, it is good to check that this code can properly handle two overrides. Is that in the unit test? https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.cc#n... base/path_service.cc:316: path_data->providers = p; This push operation is correctly locked, but it would have been better if the lock was not used during the construction of the new Provider (locks should be held for the least possible period of time... and try not to call other functions while holding a lock). https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.h File base/path_service.h (right): https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.h#ne... base/path_service.h:50: // was an override to remove or false if none was present. nit: If this is meant to only be used for testing, then you should probably comment about such. It is nicer to make this method private, and then friend the tests that use it, so you can enforce it. https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.h#ne... base/path_service.h:69: static bool GetFromCache(int key, FilePath* path); Especially if you're going to use the "test is friend" approach to restrict access to your function, you should move these functions into anonymous namespaces in the implementation (as per Chromium style guide).
I think RemoveOverride is going to be useful and is not more dangerous than the Override function itself. It actually has some nice properties which are harder to achieve through Override alone and that is the stacking of paths where one path is actually an alias to a subdir in another path. Overrides break this semantic but then the ability to remove them restores that. I have seen a few tests that override plugin paths and this is an example of a path that is a subdir of another path variable. Especially unit tests are very vulnerable to overrides because they all live in the same process so an override in one test can influence another tests and therefore reverting changes to the overrides is a good idea. I have a follow up CL that adds a ScopedPathOverride class intended to give tests an easy way to temporarily override paths and restore them upon exiting some scope as a clean way to testing code that depends on path constants. https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.cc File base/path_service.cc (right): https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.cc#n... base/path_service.cc:168: *result = it->second; On 2012/09/15 02:01:20, jar wrote: > This is (I think) the one place they *could* (should?) have put an override into > the cache.... but it does not get done. > > It might be simpler to never use the cache for overridden values. > > To be cleaner, it is then probably nicer to check for the override while we have > the lock and are checking for the cache, rather than acquiring the lock twice. > This way we don't have to copy the override into a second map. Absolutely. What happens now seems to be that only the last override ever stays in the cache and all others are always taken out of the overrides array. I have uploaded now a heavily rewritten version that actually hides GetFrom* + AddToCache in the internal implementation portion of this file and changes their contract to make them expect the structre is locked from the caller. This allows us to make much less locks than needed and properly cache all paths. Given that this is bigger change I decided to ask Brett to have a look at it too because I think he wrote big portions of this code or will know who to delegate this. https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.cc#n... base/path_service.cc:207: Provider* provider = path_data->providers; On 2012/09/15 02:01:20, jar wrote: > This is probably racy, as the contents of path_data->provider may arrive > asynchronously, and worse yet, there are no acquire and release semantics. As a > result, we might get the pointer intact... but what it points to might not be > intact. > > The acquisition of the start of this list should be done under a lock, although > it is true that we can iterate over the list without a lock. > > The corresponding lock is present at the end of the file (where we do the > prepend). Theoretically you are right but practically the list is rather static. If new providers should arrive it only happens in the initialization path which happens before we spawn the threads. Only overrides might happen later in the lifetime of the process. And even that happens mostly very early at start up because as the comment says changes to the overrides might be very unexpected to the clients and should not be considered lightly. That said I would be confortable leaving this assignment without a lock. https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.cc#n... base/path_service.cc:224: // static On 2012/09/15 02:01:20, jar wrote: > I personally like this style... but it is a gratuitous style change to this > file. You should just leave the existing style intact. Actually the file is already mixed style (see line 185). I think in that case unifying the style is still a step in the right direction :) https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.cc#n... base/path_service.cc:263: path_data->cache[key] = file_path; On 2012/09/15 02:01:20, jar wrote: > It is interesting that the directly implent into the cache when they establish > the override. This is a little suspicious, but could have just been premature > optimization. > Note that the cache.clear() above would have undone other "premature > optimization" done for other overrides. > > Given the odd performance, it is good to check that this code can properly > handle two overrides. Is that in the unit test? I agree. I added a new test to verify that. https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.cc#n... base/path_service.cc:316: path_data->providers = p; On 2012/09/15 02:01:20, jar wrote: > This push operation is correctly locked, but it would have been better if the > lock was not used during the construction of the new Provider (locks should be > held for the least possible period of time... and try not to call other > functions while holding a lock). Done.
https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.cc File base/path_service.cc (right): https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.cc#n... base/path_service.cc:207: Provider* provider = path_data->providers; On 2012/09/17 16:30:40, pastarmovj wrote: > On 2012/09/15 02:01:20, jar wrote: > > This is probably racy, as the contents of path_data->provider may arrive > > asynchronously, and worse yet, there are no acquire and release semantics. As > a > > result, we might get the pointer intact... but what it points to might not be > > intact. > > > > The acquisition of the start of this list should be done under a lock, > although > > it is true that we can iterate over the list without a lock. > > > > The corresponding lock is present at the end of the file (where we do the > > prepend). > > Theoretically you are right but practically the list is rather static. If new > providers should arrive it only happens in the initialization path which happens > before we spawn the threads. Only overrides might happen later in the lifetime > of the process. And even that happens mostly very early at start up because as > the comment says changes to the overrides might be very unexpected to the > clients and should not be considered lightly. That said I would be confortable > leaving this assignment without a lock. The comment is wrong. The fact that we use a lock to pre-pend makes it clear we are potentially mutli-threaded when doing so, and hence we should use a lock to read this to start an iterator. Unless you can argue that this is all done while single threaded (which is not the argument in the comment), this should be fixed. https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.h File base/path_service.h (right): https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.h#ne... base/path_service.h:69: static bool GetFromCache(int key, FilePath* path); It is good that you pulled these static helper methods from here... but I was hoping to to see the RemoveOverride() pushed into the private section, and then use a friend-test to allow tests to tickle this stuff. On 2012/09/15 02:01:20, jar wrote: > Especially if you're going to use the "test is friend" approach to restrict > access to your function, you should move these functions into anonymous > namespaces in the implementation (as per Chromium style guide). https://chromiumcodereview.appspot.com/10909228/diff/3006/base/path_service.cc File base/path_service.cc (right): https://chromiumcodereview.appspot.com/10909228/diff/3006/base/path_service.c... base/path_service.cc:162: bool GetFromOverrides(int key, PathData* path_data, FilePath* result) { A good readability pattern is to preface helper functions with the word "Locked" if the lock was acquired before calling. In this case, it would be: LockedGetFromOverrides(). The same is true for: LockedAddToCache(); and LockedGetFromeCache(); etc. https://chromiumcodereview.appspot.com/10909228/diff/3006/base/path_service_u... File base/path_service_unittest.cc (right): https://chromiumcodereview.appspot.com/10909228/diff/3006/base/path_service_u... base/path_service_unittest.cc:115: // Check if multiple overrides can co-excist. nit: excist --> exist
Thanks for the extensive review! Cleaned up the code according to your suggestions. PTAL. https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.cc File base/path_service.cc (right): https://chromiumcodereview.appspot.com/10909228/diff/1/base/path_service.cc#n... base/path_service.cc:207: Provider* provider = path_data->providers; On 2012/09/17 17:06:08, jar wrote: > On 2012/09/17 16:30:40, pastarmovj wrote: > > On 2012/09/15 02:01:20, jar wrote: > > > This is probably racy, as the contents of path_data->provider may arrive > > > asynchronously, and worse yet, there are no acquire and release semantics. > As > > a > > > result, we might get the pointer intact... but what it points to might not > be > > > intact. > > > > > > The acquisition of the start of this list should be done under a lock, > > although > > > it is true that we can iterate over the list without a lock. > > > > > > The corresponding lock is present at the end of the file (where we do the > > > prepend). > > > > Theoretically you are right but practically the list is rather static. If new > > providers should arrive it only happens in the initialization path which > happens > > before we spawn the threads. Only overrides might happen later in the lifetime > > of the process. And even that happens mostly very early at start up because as > > the comment says changes to the overrides might be very unexpected to the > > clients and should not be considered lightly. That said I would be confortable > > leaving this assignment without a lock. > > The comment is wrong. The fact that we use a lock to pre-pend makes it clear we > are potentially mutli-threaded when doing so, and hence we should use a lock to > read this to start an iterator. > > Unless you can argue that this is all done while single threaded (which is not > the argument in the comment), this should be fixed. > > Done. https://chromiumcodereview.appspot.com/10909228/diff/3006/base/path_service.cc File base/path_service.cc (right): https://chromiumcodereview.appspot.com/10909228/diff/3006/base/path_service.c... base/path_service.cc:162: bool GetFromOverrides(int key, PathData* path_data, FilePath* result) { On 2012/09/17 17:06:08, jar wrote: > A good readability pattern is to preface helper functions with the word "Locked" > if the lock was acquired before calling. In this case, it would be: > > LockedGetFromOverrides(). > > The same is true for: > > LockedAddToCache(); > and > LockedGetFromeCache(); > > etc. Done. https://chromiumcodereview.appspot.com/10909228/diff/3006/base/path_service_u... File base/path_service_unittest.cc (right): https://chromiumcodereview.appspot.com/10909228/diff/3006/base/path_service_u... base/path_service_unittest.cc:115: // Check if multiple overrides can co-excist. On 2012/09/17 17:06:08, jar wrote: > nit: excist --> exist Done.
https://chromiumcodereview.appspot.com/10909228/diff/17001/base/path_service.cc File base/path_service.cc (right): https://chromiumcodereview.appspot.com/10909228/diff/17001/base/path_service.... base/path_service.cc:146: path_data->cache[key] = path; This line of code (which is the whole function), seems small enough and clear enough that it should be inlined explicitly (no need for a big comment about it, etc.). https://chromiumcodereview.appspot.com/10909228/diff/17001/base/path_service.... base/path_service.cc:219: LockedAddToCache(key, path, path_data); nit: Move these two line down to between lines 222 and 223, and there will be no need for curlies.
Addressed the last round of comments. https://chromiumcodereview.appspot.com/10909228/diff/17001/base/path_service.cc File base/path_service.cc (right): https://chromiumcodereview.appspot.com/10909228/diff/17001/base/path_service.... base/path_service.cc:146: path_data->cache[key] = path; On 2012/09/18 16:47:17, jar wrote: > This line of code (which is the whole function), seems small enough and clear > enough that it should be inlined explicitly (no need for a big comment about it, > etc.). Done. https://chromiumcodereview.appspot.com/10909228/diff/17001/base/path_service.... base/path_service.cc:219: LockedAddToCache(key, path, path_data); On 2012/09/18 16:47:17, jar wrote: > nit: Move these two line down to between lines 222 and 223, and there will be no > need for curlies. Done.
https://chromiumcodereview.appspot.com/10909228/diff/19006/base/path_service.cc File base/path_service.cc (right): https://chromiumcodereview.appspot.com/10909228/diff/19006/base/path_service.... base/path_service.cc:179: Provider* provider = NULL; nit: move to in front of line 184, closest to first use that we can be. https://chromiumcodereview.appspot.com/10909228/diff/19006/base/path_service.... base/path_service.cc:198: // search providers for the requested path nit: Probably delete this, but you could put it near line 193 if you wanted. https://chromiumcodereview.appspot.com/10909228/diff/19006/base/path_service.... base/path_service.cc:199: // NOTE: it should be safe to iterate here without the lock Delete the outdated comments on line 199 and 200.
https://chromiumcodereview.appspot.com/10909228/diff/19006/base/path_service.cc File base/path_service.cc (right): https://chromiumcodereview.appspot.com/10909228/diff/19006/base/path_service.... base/path_service.cc:179: Provider* provider = NULL; On 2012/09/20 01:51:19, jar wrote: > nit: move to in front of line 184, closest to first use that we can be. Done. https://chromiumcodereview.appspot.com/10909228/diff/19006/base/path_service.... base/path_service.cc:198: // search providers for the requested path On 2012/09/20 01:51:19, jar wrote: > nit: Probably delete this, but you could put it near line 193 if you wanted. Done. https://chromiumcodereview.appspot.com/10909228/diff/19006/base/path_service.... base/path_service.cc:199: // NOTE: it should be safe to iterate here without the lock On 2012/09/20 01:51:19, jar wrote: > Delete the outdated comments on line 199 and 200. I think it is good to have a comment why this is not protected by the lock. So I slightly modified the comment but left it in.
lgtm https://chromiumcodereview.appspot.com/10909228/diff/19006/base/path_service.cc File base/path_service.cc (right): https://chromiumcodereview.appspot.com/10909228/diff/19006/base/path_service.... base/path_service.cc:199: // NOTE: it should be safe to iterate here without the lock On 2012/09/20 09:32:19, pastarmovj wrote: > On 2012/09/20 01:51:19, jar wrote: > > Delete the outdated comments on line 199 and 200. > > I think it is good to have a comment why this is not protected by the lock. So I > slightly modified the comment but left it in. Good point. I'm fine with that.
Thanks for the review! Brett are you ok with this change too?
jar's review should be sufficient unless there's some specific question you have for me.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/10909228/27001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/10909228/27001
Change committed as 158842 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
