Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1001673002:
Add Locking calls to file_io.h plus implementations and test (Closed)
DescriptionAdd Locking calls to file_io.h plus implementations and test
Towards removing use of open() with O_EXLOCK/O_SHLOCK in code
used on non-BSD.
Adds simple Thread abstraction to util/test.
Includes mini_chromium roll with:
56dd2883170d0df0ec89af0e7862af3f9aaa9be6 Fix import of atomicops for Windows
886592fd6677615c54c4156bb2f2edb5d547ba6c Export SystemErrorCodeToString
R=mark@chromium.org, rsesek@chromium.org
BUG=crashpad:1, crashpad:13
Committed: https://chromium.googlesource.com/crashpad/crashpad/+/79ae055e50b1acd5050795a7ac9b5bc0afcee2d9
Patch Set 1 #Patch Set 2 : posix #Patch Set 3 : . #Patch Set 4 : . #
Total comments: 33
Patch Set 5 : partial revisions #Patch Set 6 : parameterize test #
Total comments: 41
Patch Set 7 : fixes, posix only #Patch Set 8 : windows #Patch Set 9 : doc comments #Patch Set 10 : add deps roll #
Total comments: 1
Patch Set 11 : ws #
Total comments: 29
Patch Set 12 : fixes #
Total comments: 11
Patch Set 13 : release load #
Messages
Total messages: 20 (2 generated)
scottmg@chromium.org changed reviewers: + rsesek@chromium.org
https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_posix.cc File util/file/file_io_posix.cc (right): https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_posix... util/file/file_io_posix.cc:106: int rv = HANDLE_EINTR(flock(file, operation)); Since we’re moving away from the BSD-specific O_SHLOCK/O_EXLOCK, we should also move away from the BSD-derived flock() interface altogether, and use the more reliable, well-specified, and portable POSIX fcntl() F_SETLKW/F_UNLCK lock interface. The differences are minor on Mac OS X and BSDs, but on other systems including Linux, fcntl() locks are better-supported. One major difference is that flock() support over NFS varies from nonexistent to spotty. The interface with the best shot at working over NFS is the fcntl() lock. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.cc File util/file/file_io_test.cc (right): https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:30: shared_file, FileWriteMode::kReuseOrCreate, FilePermissions::kOwnerOnly)); The first one should use kCreateOrFail. Same on lines 43 and 111. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:43: shared_file, FileWriteMode::kReuseOrCreate, FilePermissions::kOwnerOnly)); ASSERT_NE(handle1, kInvalidFileHandle) after this, like in FileIO.FileShareMode? Same for handle2. Also in FileIO.ExclusiveExcludes. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:55: #if defined(OS_POSIX) OK for now. Robert and I were talking about needing a threading primitive today. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:60: FileHandle file; ScopedFileHandle? https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:73: EXPECT_TRUE(LoggingLockFile(info->file, FileLocking::kExclusive)); You haven’t done anything to test that an exclusive lock also blocks an attempt to get a shared lock, or that one or more shared locks block an attempt to get an exclusive lock. Maybe you could cheaply parameterize this test so that for (main_thread_lock_type, other_threads_lock_type) you try all of (exclusive, exclusive) (what you’ve done), (exclusive, shared), and (shared, exclusive). https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:74: *info->shared_counter += 1; For the case where the the other threads will be using shared locks, this would need to become an atomic increment. Bringing base/atomicops.h into mini_chromium should be fine, that’s a reasonable interface with strong implementations. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:88: ASSERT_EQ(0, rv) << "pthread_create"; From util/test/errors.h, use ErrnoMessage, like ASSERT_EQ(0, rv) << ErrnoMessage(rv, "pthread_create"); Same on line 98. Maybe you’d want a similar ErrorMessage() for Windows too. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:118: int iterations = 0; expected_iterations makes it clearer. If shared_counter becomes actual_iterations, it’s better still. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:119: for (int index = 0; index < kThreads; ++index) { #include "base/basictypes.h" and use arraysize(info) here and in the other loops. Then you can just declare it as info[10] and get rid of kThreads. You could even use a range-based for here, except you want index for other purposes. But you still could use range-based for in the other loops. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:139: EXPECT_EQ(shared_counter, iterations); These arguments are swapped. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:142: EXPECT_TRUE(LoggingCloseFile(info[index].file)); This could have happened in ThreadMain() instead of having a third loop here.
https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io.h#new... util/file/file_io.h:194: //! (that is, no CRLF translation). On Windows, the file is opened for sharing, Are files normally exclusive on Windows? Do we want most files opened as sharable if so? https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_posix.cc File util/file/file_io_posix.cc (right): https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_posix... util/file/file_io_posix.cc:105: int operation = locking == FileLocking::kShared ? LOCK_SH : LOCK_EX; nit: () around the condition https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.cc File util/file/file_io_test.cc (right): https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:55: #if defined(OS_POSIX) On 2015/03/12 04:47:42, Mark Mentovai wrote: > OK for now. Robert and I were talking about needing a threading primitive today. I think it's probably time to create this abstraction (at least for use in test code, I think this is the third instance of something like this). https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_win.cc File util/file/file_io_win.cc (right): https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_win.c... util/file/file_io_win.cc:119: locking == FileLocking::kExclusive ? LOCKFILE_EXCLUSIVE_LOCK : 0; nit: () around condition
Sorry for the delay between revisions, hair fires on M41 push. I'm confused by the fcntl API. The initial lock doesn't seem to exclude and fails as ../../util/file/file_io_test.cc:124: Failure Value of: actual_iterations Actual: 203 Expected: 0 Is there anything obviously incorrect in my usage in the #if 0 blocks in file_io_posix.cc? https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io.h#new... util/file/file_io.h:194: //! (that is, no CRLF translation). On Windows, the file is opened for sharing, On 2015/03/12 15:28:03, Robert Sesek wrote: > Are files normally exclusive on Windows? Do we want most files opened as > sharable if so? I don't think there's an expected default. I just made them exclusive to start with so that it'd be clear when something required sharing. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_posix.cc File util/file/file_io_posix.cc (right): https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_posix... util/file/file_io_posix.cc:105: int operation = locking == FileLocking::kShared ? LOCK_SH : LOCK_EX; On 2015/03/12 15:28:03, Robert Sesek wrote: > nit: () around the condition Done. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_posix... util/file/file_io_posix.cc:106: int rv = HANDLE_EINTR(flock(file, operation)); On 2015/03/12 04:47:42, Mark Mentovai wrote: > Since we’re moving away from the BSD-specific O_SHLOCK/O_EXLOCK, we should also > move away from the BSD-derived flock() interface altogether, and use the more > reliable, well-specified, and portable POSIX fcntl() F_SETLKW/F_UNLCK lock > interface. > > The differences are minor on Mac OS X and BSDs, but on other systems including > Linux, fcntl() locks are better-supported. > > One major difference is that flock() support over NFS varies from nonexistent to > spotty. The interface with the best shot at working over NFS is the fcntl() > lock. I tried to do this, but I haven't been able to get exclusive fcntl locks to work. The #if 0'd code doesn't work, the #else code does, but I've stared at `man fcntl` for a while now and I'm not sure why. Can either of you enlighten me? https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.cc File util/file/file_io_test.cc (right): https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:30: shared_file, FileWriteMode::kReuseOrCreate, FilePermissions::kOwnerOnly)); On 2015/03/12 04:47:42, Mark Mentovai wrote: > The first one should use kCreateOrFail. Same on lines 43 and 111. Done. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:43: shared_file, FileWriteMode::kReuseOrCreate, FilePermissions::kOwnerOnly)); On 2015/03/12 04:47:42, Mark Mentovai wrote: > ASSERT_NE(handle1, kInvalidFileHandle) after this, like in FileIO.FileShareMode? > Same for handle2. Also in FileIO.ExclusiveExcludes. Done. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:55: #if defined(OS_POSIX) On 2015/03/12 15:28:03, Robert Sesek wrote: > On 2015/03/12 04:47:42, Mark Mentovai wrote: > > OK for now. Robert and I were talking about needing a threading primitive > today. > > I think it's probably time to create this abstraction (at least for use in test > code, I think this is the third instance of something like this). Added some version of this to util/test/thread.h. I didn't convert other usages pending discussion on interface and/or deferral to future CLs. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:60: FileHandle file; On 2015/03/12 04:47:42, Mark Mentovai wrote: > ScopedFileHandle? Done. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:73: EXPECT_TRUE(LoggingLockFile(info->file, FileLocking::kExclusive)); On 2015/03/12 04:47:42, Mark Mentovai wrote: > You haven’t done anything to test that an exclusive lock also blocks an attempt > to get a shared lock, or that one or more shared locks block an attempt to get > an exclusive lock. Maybe you could cheaply parameterize this test so that for > (main_thread_lock_type, other_threads_lock_type) you try all of (exclusive, > exclusive) (what you’ve done), (exclusive, shared), and (shared, exclusive). Done. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:74: *info->shared_counter += 1; On 2015/03/12 04:47:42, Mark Mentovai wrote: > For the case where the the other threads will be using shared locks, this would > need to become an atomic increment. Bringing base/atomicops.h into mini_chromium > should be fine, that’s a reasonable interface with strong implementations. Done. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:88: ASSERT_EQ(0, rv) << "pthread_create"; On 2015/03/12 04:47:42, Mark Mentovai wrote: > From util/test/errors.h, use ErrnoMessage, like > > ASSERT_EQ(0, rv) << ErrnoMessage(rv, "pthread_create"); > > Same on line 98. Maybe you’d want a similar ErrorMessage() for Windows too. Done. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:118: int iterations = 0; On 2015/03/12 04:47:42, Mark Mentovai wrote: > expected_iterations makes it clearer. If shared_counter becomes > actual_iterations, it’s better still. Done. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:119: for (int index = 0; index < kThreads; ++index) { On 2015/03/12 04:47:42, Mark Mentovai wrote: > #include "base/basictypes.h" and use arraysize(info) here and in the other > loops. Then you can just declare it as info[10] and get rid of kThreads. You > could even use a range-based for here, except you want index for other purposes. > But you still could use range-based for in the other loops. Done. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:139: EXPECT_EQ(shared_counter, iterations); On 2015/03/12 04:47:42, Mark Mentovai wrote: > These arguments are swapped. Done. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_test.... util/file/file_io_test.cc:142: EXPECT_TRUE(LoggingCloseFile(info[index].file)); On 2015/03/12 04:47:42, Mark Mentovai wrote: > This could have happened in ThreadMain() instead of having a third loop here. Removed via ScopedFileHandle. https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_win.cc File util/file/file_io_win.cc (right): https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_win.c... util/file/file_io_win.cc:119: locking == FileLocking::kExclusive ? LOCKFILE_EXCLUSIVE_LOCK : 0; On 2015/03/12 15:28:03, Robert Sesek wrote: > nit: () around condition Done.
Glad to see this come back! https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_posix.cc File util/file/file_io_posix.cc (right): https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_posix... util/file/file_io_posix.cc:106: int rv = HANDLE_EINTR(flock(file, operation)); On 2015/03/19 22:06:19, scottmg wrote: > On 2015/03/12 04:47:42, Mark Mentovai wrote: > > Since we’re moving away from the BSD-specific O_SHLOCK/O_EXLOCK, we should > also > > move away from the BSD-derived flock() interface altogether, and use the more > > reliable, well-specified, and portable POSIX fcntl() F_SETLKW/F_UNLCK lock > > interface. > > > > The differences are minor on Mac OS X and BSDs, but on other systems including > > Linux, fcntl() locks are better-supported. > > > > One major difference is that flock() support over NFS varies from nonexistent > to > > spotty. The interface with the best shot at working over NFS is the fcntl() > > lock. > > I tried to do this, but I haven't been able to get exclusive fcntl locks to > work. The #if 0'd code doesn't work, the #else code does, but I've stared at > `man fcntl` for a while now and I'm not sure why. Can either of you enlighten > me? I think you got it right, but it seems that fcntl() locks belong to the process, not the fd. Oh well. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io.h#new... util/file/file_io.h:81: //! \brief Determines the locking mode that LoggingLockFile uses. Nit: parentheses on function names. flock() below, too. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io.h#new... util/file/file_io.h:206: //! \brief Locks the given \a file similar to `fcntl` `F_SETLKW`. If we’re going back to flock(), don’t forget to update this. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io.h#new... util/file/file_io.h:212: //! If the file is \a locking is FileLocking::kShared, \a file must have been Too many “is”? https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io.h#new... util/file/file_io.h:217: //! \param[in] locking Controls whether the lock is shared reader lock, or an is _a_ shared? https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io.h#new... util/file/file_io.h:225: //! It is an error to attempt to unlock a file that was not previously locked. If it’s true on Windows too, you can say that closing FileHandle will release the lock without calling this function. In that case, also add a test to make sure that this works correctly. If that’s not true on Windows, say that this function must be called and that it’s not sufficient to close the FileHandle to get the lock released. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_posix.cc File util/file/file_io_posix.cc (right): https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_posix... util/file/file_io_posix.cc:128: int rv = HANDLE_EINTR(flock(file, LOCK_UN)); Shouldn’t need to HANDLE_EINTR for an unlock (although it’s harmless if you want to be really paranoid). https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_test.cc File util/file/file_io_test.cc (right): https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_test.... util/file/file_io_test.cc:76: base::subtle::Barrier_AtomicIncrement(info->actual_iterations, 1); I don’t think you need a memory barrier here. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_test.... util/file/file_io_test.cc:103: base::subtle::Atomic32 actual_iterations = 0; I’m not sure if the best practice for initialization is to use direct assignment like this or to declare it and then do an Acquire_Store() immediately afterwards. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_test.... util/file/file_io_test.cc:107: base::subtle::Atomic32 expected_iterations = 0; This is not actually used for atomicity, it’s just typed that way for compatibility with actual_iterations. Can you add a comment to that effect, because otherwise it’s a red flag to see it being manipulated with direct operations like += instead of atomic operations. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_test.... util/file/file_io_test.cc:124: EXPECT_EQ(0, actual_iterations); This one should have a barrier, so technically it should be Atomic32 result = Release_Load(&actual_iterations);, and then EXPECT on release. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_win.cc File util/file/file_io_win.cc (right): https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_win.c... util/file/file_io_win.cc:138: PLOG(ERROR) << "LockFileEx"; UnlockFileEx https://codereview.chromium.org/1001673002/diff/90009/util/test/errors.cc File util/test/errors.cc (right): https://codereview.chromium.org/1001673002/diff/90009/util/test/errors.cc#new... util/test/errors.cc:57: base::SystemErrorCodeToString(GetLastError()).c_str()); This is not declared by mini_chromium’s logging.h. Yet. You can add it there and bring the DEPS roll into this change. https://codereview.chromium.org/1001673002/diff/90009/util/test/errors.h File util/test/errors.h (right): https://codereview.chromium.org/1001673002/diff/90009/util/test/errors.h#newc... util/test/errors.h:75: //! the PLOG() formatting in base. PLOG() doesn’t get Doxygenated, so `PLOG()` to make it stand out in the generated documentation. https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h File util/test/thread.h (right): https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h#newc... util/test/thread.h:18: #include "build/build_config.h" build > base https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h#newc... util/test/thread.h:33: Thread() : entry_(nullptr), data_(nullptr), thread_(nullptr) {} nullptr isn’t a valid pthread_t on all systems. 0 or a static const kInvalidThreadHandle = 0 would work. Nothing requires 0 to be an invalid pthread_t, although it’s true on all of the systems we will care about. POSIX is introducing PTHREAD_NULL, but it’s not in the spec yet and none of our systems have it yet either. https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h#newc... util/test/thread.h:34: void Start(void (*entry)(T*), T* data); The function pointer is a very C-like interface, with a thin C++ template above it to try to mask it. How about if instead, you made Thread a base class that calls an unimplemented virtual Run() method? Then all of the data is encapsulated in subclass objects, so you don’t need to deal with the data argument pointer. That also lets the implementation move to a .cc file and avoids having multiple copies of the same thing each time the template gets instantiated with a different T. You could also separate _win.cc from _posix.cc.
P.S. https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h File util/test/thread.h (right): https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h#newc... util/test/thread.h:31: class Thread { Provide at least a //! \brief for the class and all of the public methods. For Join(), it’d be good to call out which threads it can validly be called on: any thread other than the one that was started vs. just the thread that called Start(). https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h#newc... util/test/thread.h:33: Thread() : entry_(nullptr), data_(nullptr), thread_(nullptr) {} ~Thread() should either EXPECT that if Start() was called, Join() was also called. OR it should Join() if there was a Start() without a Join(). I prefer the former. This will catch bad uses of Thread where they happen, so that an errant thread left alive in one test can’t ruin the state for future tests. https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h#newc... util/test/thread.h:34: void Start(void (*entry)(T*), T* data); I wrote: > How about if instead, you made Thread a base class that calls an unimplemented > virtual Run() method? Then all of the data is encapsulated in subclass objects, > so you don’t need to deal with the data argument pointer. Run() would be a pretty bad name, but you get the idea. https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h#newc... util/test/thread.h:35: void Join(); Similarly, Start() should EXPECT that it hasn’t already been called.
https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_test.cc File util/file/file_io_test.cc (right): https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_test.... util/file/file_io_test.cc:103: base::subtle::Atomic32 actual_iterations = 0; On 2015/03/20 15:03:55, Mark Mentovai wrote: > I’m not sure if the best practice for initialization is to use direct assignment > like this or to declare it and then do an Acquire_Store() immediately > afterwards. Most instances in Chromium seem to initialize without an Acquire.
https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_posix.cc File util/file/file_io_posix.cc (right): https://codereview.chromium.org/1001673002/diff/60001/util/file/file_io_posix... util/file/file_io_posix.cc:106: int rv = HANDLE_EINTR(flock(file, operation)); On 2015/03/20 15:03:54, Mark Mentovai wrote: > On 2015/03/19 22:06:19, scottmg wrote: > > On 2015/03/12 04:47:42, Mark Mentovai wrote: > > > Since we’re moving away from the BSD-specific O_SHLOCK/O_EXLOCK, we should > > also > > > move away from the BSD-derived flock() interface altogether, and use the > more > > > reliable, well-specified, and portable POSIX fcntl() F_SETLKW/F_UNLCK lock > > > interface. > > > > > > The differences are minor on Mac OS X and BSDs, but on other systems > including > > > Linux, fcntl() locks are better-supported. > > > > > > One major difference is that flock() support over NFS varies from > nonexistent > > to > > > spotty. The interface with the best shot at working over NFS is the fcntl() > > > lock. > > > > I tried to do this, but I haven't been able to get exclusive fcntl locks to > > work. The #if 0'd code doesn't work, the #else code does, but I've stared at > > `man fcntl` for a while now and I'm not sure why. Can either of you enlighten > > me? > > I think you got it right, but it seems that fcntl() locks belong to the process, > not the fd. > > Oh well. Ah, yes. It does repeatedly say process in the docs, darn. I'm going to go back to flock for now then. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io.h#new... util/file/file_io.h:81: //! \brief Determines the locking mode that LoggingLockFile uses. On 2015/03/20 15:03:55, Mark Mentovai wrote: > Nit: parentheses on function names. flock() below, too. Done. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io.h#new... util/file/file_io.h:206: //! \brief Locks the given \a file similar to `fcntl` `F_SETLKW`. On 2015/03/20 15:03:55, Mark Mentovai wrote: > If we’re going back to flock(), don’t forget to update this. Done. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io.h#new... util/file/file_io.h:212: //! If the file is \a locking is FileLocking::kShared, \a file must have been On 2015/03/20 15:03:55, Mark Mentovai wrote: > Too many “is”? Done. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io.h#new... util/file/file_io.h:217: //! \param[in] locking Controls whether the lock is shared reader lock, or an On 2015/03/20 15:03:54, Mark Mentovai wrote: > is _a_ shared? Done. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io.h#new... util/file/file_io.h:225: //! It is an error to attempt to unlock a file that was not previously locked. On 2015/03/20 15:03:54, Mark Mentovai wrote: > If it’s true on Windows too, you can say that closing FileHandle will release > the lock without calling this function. In that case, also add a test to make > sure that this works correctly. > > If that’s not true on Windows, say that this function must be called and that > it’s not sufficient to close the FileHandle to get the lock released. Unfortunately, it's semi-true on Windows. """If a process terminates with a portion of a file locked or closes a file that has outstanding locks, the locks are unlocked by the operating system. However, the time it takes for the operating system to unlock these locks depends upon available system resources.""" So, updated to say that it should be released explicitly. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_posix.cc File util/file/file_io_posix.cc (right): https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_posix... util/file/file_io_posix.cc:128: int rv = HANDLE_EINTR(flock(file, LOCK_UN)); On 2015/03/20 15:03:55, Mark Mentovai wrote: > Shouldn’t need to HANDLE_EINTR for an unlock (although it’s harmless if you want > to be really paranoid). Done. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_test.cc File util/file/file_io_test.cc (right): https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_test.... util/file/file_io_test.cc:76: base::subtle::Barrier_AtomicIncrement(info->actual_iterations, 1); On 2015/03/20 15:03:55, Mark Mentovai wrote: > I don’t think you need a memory barrier here. Done. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_test.... util/file/file_io_test.cc:103: base::subtle::Atomic32 actual_iterations = 0; On 2015/03/20 16:31:35, Robert Sesek wrote: > On 2015/03/20 15:03:55, Mark Mentovai wrote: > > I’m not sure if the best practice for initialization is to use direct > assignment > > like this or to declare it and then do an Acquire_Store() immediately > > afterwards. > > Most instances in Chromium seem to initialize without an Acquire. Left as direct assignment. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_test.... util/file/file_io_test.cc:107: base::subtle::Atomic32 expected_iterations = 0; On 2015/03/20 15:03:56, Mark Mentovai wrote: > This is not actually used for atomicity, it’s just typed that way for > compatibility with actual_iterations. Can you add a comment to that effect, > because otherwise it’s a red flag to see it being manipulated with direct > operations like += instead of atomic operations. Right. I switched it back to int to make usage less unclear as it was unnecessary anyway. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_test.... util/file/file_io_test.cc:124: EXPECT_EQ(0, actual_iterations); On 2015/03/20 15:03:55, Mark Mentovai wrote: > This one should have a barrier, so technically it should be Atomic32 result = > Release_Load(&actual_iterations);, and then EXPECT on release. Done. https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_win.cc File util/file/file_io_win.cc (right): https://codereview.chromium.org/1001673002/diff/90009/util/file/file_io_win.c... util/file/file_io_win.cc:138: PLOG(ERROR) << "LockFileEx"; On 2015/03/20 15:03:56, Mark Mentovai wrote: > UnlockFileEx Done. https://codereview.chromium.org/1001673002/diff/90009/util/test/errors.cc File util/test/errors.cc (right): https://codereview.chromium.org/1001673002/diff/90009/util/test/errors.cc#new... util/test/errors.cc:57: base::SystemErrorCodeToString(GetLastError()).c_str()); On 2015/03/20 15:03:56, Mark Mentovai wrote: > This is not declared by mini_chromium’s logging.h. Yet. You can add it there and > bring the DEPS roll into this change. I admit I had not yet tested back on Windows yet. Done. https://codereview.chromium.org/1001673002/diff/90009/util/test/errors.h File util/test/errors.h (right): https://codereview.chromium.org/1001673002/diff/90009/util/test/errors.h#newc... util/test/errors.h:75: //! the PLOG() formatting in base. On 2015/03/20 15:03:56, Mark Mentovai wrote: > PLOG() doesn’t get Doxygenated, so `PLOG()` to make it stand out in the > generated documentation. Done. https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h File util/test/thread.h (right): https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h#newc... util/test/thread.h:18: #include "build/build_config.h" On 2015/03/20 15:03:56, Mark Mentovai wrote: > build > base Done. https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h#newc... util/test/thread.h:31: class Thread { On 2015/03/20 15:42:12, Mark Mentovai wrote: > Provide at least a //! \brief for the class and all of the public methods. For > Join(), it’d be good to call out which threads it can validly be called on: any > thread other than the one that was started vs. just the thread that called > Start(). Done. https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h#newc... util/test/thread.h:33: Thread() : entry_(nullptr), data_(nullptr), thread_(nullptr) {} On 2015/03/20 15:03:56, Mark Mentovai wrote: > nullptr isn’t a valid pthread_t on all systems. 0 or a static const > kInvalidThreadHandle = 0 would work. > > Nothing requires 0 to be an invalid pthread_t, although it’s true on all of the > systems we will care about. POSIX is introducing PTHREAD_NULL, but it’s not in > the spec yet and none of our systems have it yet either. Done. https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h#newc... util/test/thread.h:33: Thread() : entry_(nullptr), data_(nullptr), thread_(nullptr) {} On 2015/03/20 15:03:56, Mark Mentovai wrote: > nullptr isn’t a valid pthread_t on all systems. 0 or a static const > kInvalidThreadHandle = 0 would work. > > Nothing requires 0 to be an invalid pthread_t, although it’s true on all of the > systems we will care about. POSIX is introducing PTHREAD_NULL, but it’s not in > the spec yet and none of our systems have it yet either. Done. https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h#newc... util/test/thread.h:34: void Start(void (*entry)(T*), T* data); On 2015/03/20 15:42:12, Mark Mentovai wrote: > I wrote: > > How about if instead, you made Thread a base class that calls an unimplemented > > virtual Run() method? Then all of the data is encapsulated in subclass > objects, > > so you don’t need to deal with the data argument pointer. > > Run() would be a pretty bad name, but you get the idea. Main()? https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h#newc... util/test/thread.h:34: void Start(void (*entry)(T*), T* data); On 2015/03/20 15:03:56, Mark Mentovai wrote: > The function pointer is a very C-like interface, with a thin C++ template above > it to try to mask it. > > How about if instead, you made Thread a base class that calls an unimplemented > virtual Run() method? Then all of the data is encapsulated in subclass objects, > so you don’t need to deal with the data argument pointer. > > That also lets the implementation move to a .cc file and avoids having multiple > copies of the same thing each time the template gets instantiated with a > different T. You could also separate _win.cc from _posix.cc. Done. https://codereview.chromium.org/1001673002/diff/90009/util/test/thread.h#newc... util/test/thread.h:35: void Join(); On 2015/03/20 15:42:12, Mark Mentovai wrote: > Similarly, Start() should EXPECT that it hasn’t already been called. Done. https://codereview.chromium.org/1001673002/diff/170001/util/file/file_io_win.cc File util/file/file_io_win.cc (right): https://codereview.chromium.org/1001673002/diff/170001/util/file/file_io_win.... util/file/file_io_win.cc:85: FILE_SHARE_READ | FILE_SHARE_WRITE, I'm actually less certain per Robert's previous comment that this would generally be expected (it would probably ideally be a user provided setting?), but in our case it works better to open and then block waiting for a lock than it does to fail the open. So I made the share mode read|write, and added a test for that behaviour.
This is really good now, the remaining things are all really minor. I think the next one’ll be it. https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io.h#ne... util/file/file_io.h:206: //! \brief Locks the given \a file similar to `flock()`. This makes it sound like you’re making a comparison between file and flock(). https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io.h#ne... util/file/file_io.h:226: //! A previously locked file should be unlocked before closing the file handle, previously-locked https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_posi... File util/file/file_io_posix.cc (right): https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_posi... util/file/file_io_posix.cc:107: PLOG_IF(ERROR, rv < 0) << "flock"; You’ll get slightly better machine code if you make the conditions on this line and the next line true inverses. The compiler won’t typically know that flock() can only return 0 or -1. Same in the unlock case. https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_test.cc File util/file/file_io_test.cc (right): https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_test... util/file/file_io_test.cc:30: Read, I’m digging these enum classes! kRead, kWrite to make constantness more obvious at use sites? https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_test... util/file/file_io_test.cc:34: void FileShareModeTest(ReadOrWrite first, ReadOrWrite second) { These were good tests to add! https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_test... util/file/file_io_test.cc:59: EXPECT_NE(handle2, kInvalidFileHandle); EXPECT_NE(handle1, handle2) also, because we’re kind of depending on that for some things. At least for the next set of tests. https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_test... util/file/file_io_test.cc:62: TEST(FileIO, FileShareModes) { Can you have a separate TEST for each of these? Like FileShareModes_Read_Read, etc. That way, when one of them fails, it’ll be obvious which variant is a problem. https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_test... util/file/file_io_test.cc:96: void Main() override { Doesn’t need to be public. private’s fine. There’s no good reason for any Tom, Dick, or Harry with a LockingTestThread object to be able to call this, other than to get it called by the base via Start(). https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_test... util/file/file_io_test.cc:104: ScopedFileHandle file; private, trailing underscores, DISALLOW_COPY_AND_ASSIGN(), default constructor, setters as needed. I get the struct heritage here, but now that this is a grown-up object with a vtable and everything, having direct access from the outside is a little unexpected. https://codereview.chromium.org/1001673002/diff/190001/util/test/thread.cc File util/test/thread.cc (left): https://codereview.chromium.org/1001673002/diff/190001/util/test/thread.cc#ol... util/test/thread.cc:15: #include "util/test/scoped_temp_dir.h" --no-find-copies https://codereview.chromium.org/1001673002/diff/190001/util/test/thread.h File util/test/thread.h (right): https://codereview.chromium.org/1001673002/diff/190001/util/test/thread.h#new... util/test/thread.h:45: protected: private’s probably fine. https://codereview.chromium.org/1001673002/diff/190001/util/test/thread.h#new... util/test/thread.h:48: virtual void Main() = 0; ThreadMain()? https://codereview.chromium.org/1001673002/diff/190001/util/test/thread_posix.cc File util/test/thread_posix.cc (right): https://codereview.chromium.org/1001673002/diff/190001/util/test/thread_posix... util/test/thread_posix.cc:24: EXPECT_FALSE(platform_thread_); ASSERT on this and the one at the top of Join(). Same in the _win.cc. https://codereview.chromium.org/1001673002/diff/190001/util/test/thread_posix... util/test/thread_posix.cc:32: ASSERT_EQ(0, rv) << ErrnoMessage(rv, "pthread_join"); EXPECT like you did in the _win.cc, so that you still zero out platform_thread_ if it was a bad thread. Or leave this at ASSERT if you like that better and let there be another failure in the destructor. I don’t have a strong preference between EXPECT and ASSERT for this line, but the _posix.cc and _win.cc should match. https://codereview.chromium.org/1001673002/diff/190001/util/test/thread_win.cc File util/test/thread_win.cc (right): https://codereview.chromium.org/1001673002/diff/190001/util/test/thread_win.c... util/test/thread_win.cc:27: ASSERT_NE(nullptr, platform_thread_) << ErrorMessage("CreateThread"); ASSERT_TRUE(platform_thread)
Patchset #12 (id:210001) has been deleted
Thanks https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io.h File util/file/file_io.h (right): https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io.h#ne... util/file/file_io.h:206: //! \brief Locks the given \a file similar to `flock()`. On 2015/03/20 22:11:36, Mark Mentovai wrote: > This makes it sound like you’re making a comparison between file and flock(). Done. https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io.h#ne... util/file/file_io.h:226: //! A previously locked file should be unlocked before closing the file handle, On 2015/03/20 22:11:36, Mark Mentovai wrote: > previously-locked Done. https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_posi... File util/file/file_io_posix.cc (right): https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_posi... util/file/file_io_posix.cc:107: PLOG_IF(ERROR, rv < 0) << "flock"; On 2015/03/20 22:11:36, Mark Mentovai wrote: > You’ll get slightly better machine code if you make the conditions on this line > and the next line true inverses. The compiler won’t typically know that flock() > can only return 0 or -1. Same in the unlock case. Done. https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_test.cc File util/file/file_io_test.cc (right): https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_test... util/file/file_io_test.cc:30: Read, On 2015/03/20 22:11:37, Mark Mentovai wrote: > I’m digging these enum classes! kRead, kWrite to make constantness more obvious > at use sites? Done. https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_test... util/file/file_io_test.cc:59: EXPECT_NE(handle2, kInvalidFileHandle); On 2015/03/20 22:11:36, Mark Mentovai wrote: > EXPECT_NE(handle1, handle2) also, because we’re kind of depending on that for > some things. At least for the next set of tests. Done. https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_test... util/file/file_io_test.cc:62: TEST(FileIO, FileShareModes) { On 2015/03/20 22:11:37, Mark Mentovai wrote: > Can you have a separate TEST for each of these? Like FileShareModes_Read_Read, > etc. That way, when one of them fails, it’ll be obvious which variant is a > problem. Done. https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_test... util/file/file_io_test.cc:96: void Main() override { On 2015/03/20 22:11:37, Mark Mentovai wrote: > Doesn’t need to be public. private’s fine. There’s no good reason for any Tom, > Dick, or Harry with a LockingTestThread object to be able to call this, other > than to get it called by the base via Start(). Done. https://codereview.chromium.org/1001673002/diff/190001/util/file/file_io_test... util/file/file_io_test.cc:104: ScopedFileHandle file; On 2015/03/20 22:11:37, Mark Mentovai wrote: > private, trailing underscores, DISALLOW_COPY_AND_ASSIGN(), default constructor, > setters as needed. I get the struct heritage here, but now that this is a > grown-up object with a vtable and everything, having direct access from the > outside is a little unexpected. Done. https://codereview.chromium.org/1001673002/diff/190001/util/test/thread.cc File util/test/thread.cc (left): https://codereview.chromium.org/1001673002/diff/190001/util/test/thread.cc#ol... util/test/thread.cc:15: #include "util/test/scoped_temp_dir.h" On 2015/03/20 22:11:37, Mark Mentovai wrote: > --no-find-copies Done. https://codereview.chromium.org/1001673002/diff/190001/util/test/thread.h File util/test/thread.h (right): https://codereview.chromium.org/1001673002/diff/190001/util/test/thread.h#new... util/test/thread.h:45: protected: On 2015/03/20 22:11:37, Mark Mentovai wrote: > private’s probably fine. Done. https://codereview.chromium.org/1001673002/diff/190001/util/test/thread.h#new... util/test/thread.h:48: virtual void Main() = 0; On 2015/03/20 22:11:37, Mark Mentovai wrote: > ThreadMain()? Done. https://codereview.chromium.org/1001673002/diff/190001/util/test/thread_posix.cc File util/test/thread_posix.cc (right): https://codereview.chromium.org/1001673002/diff/190001/util/test/thread_posix... util/test/thread_posix.cc:24: EXPECT_FALSE(platform_thread_); On 2015/03/20 22:11:37, Mark Mentovai wrote: > ASSERT on this and the one at the top of Join(). Same in the _win.cc. Done. https://codereview.chromium.org/1001673002/diff/190001/util/test/thread_posix... util/test/thread_posix.cc:32: ASSERT_EQ(0, rv) << ErrnoMessage(rv, "pthread_join"); On 2015/03/20 22:11:37, Mark Mentovai wrote: > EXPECT like you did in the _win.cc, so that you still zero out platform_thread_ > if it was a bad thread. Or leave this at ASSERT if you like that better and let > there be another failure in the destructor. I don’t have a strong preference > between EXPECT and ASSERT for this line, but the _posix.cc and _win.cc should > match. Done. https://codereview.chromium.org/1001673002/diff/190001/util/test/thread_win.cc File util/test/thread_win.cc (right): https://codereview.chromium.org/1001673002/diff/190001/util/test/thread_win.c... util/test/thread_win.cc:27: ASSERT_NE(nullptr, platform_thread_) << ErrorMessage("CreateThread"); On 2015/03/20 22:11:37, Mark Mentovai wrote: > ASSERT_TRUE(platform_thread) Done.
LGTM. This is really solid now. https://codereview.chromium.org/1001673002/diff/230001/util/file/file_io_test.cc File util/file/file_io_test.cc (right): https://codereview.chromium.org/1001673002/diff/230001/util/file/file_io_test... util/file/file_io_test.cc:189: EXPECT_EQ(expected_iterations, actual_iterations); Release_Load into result again and use that instead of actual_iterations.
https://codereview.chromium.org/1001673002/diff/230001/util/file/file_io_test.cc File util/file/file_io_test.cc (right): https://codereview.chromium.org/1001673002/diff/230001/util/file/file_io_test... util/file/file_io_test.cc:189: EXPECT_EQ(expected_iterations, actual_iterations); On 2015/03/20 22:41:46, Mark Mentovai wrote: > Release_Load into result again and use that instead of actual_iterations. Done.
Message was sent while issue was closed.
Committed patchset #13 (id:250001) manually as 79ae055e50b1acd5050795a7ac9b5bc0afcee2d9 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/1001673002/diff/230001/util/file/file_io_posi... File util/file/file_io_posix.cc (right): https://codereview.chromium.org/1001673002/diff/230001/util/file/file_io_posi... util/file/file_io_posix.cc:105: int operation = locking == FileLocking::kShared ? LOCK_SH : LOCK_EX; nit: lost () around the expr https://codereview.chromium.org/1001673002/diff/230001/util/file/file_io_posi... util/file/file_io_posix.cc:106: int rv = HANDLE_EINTR(flock(file, operation)); Drop HANDLE_EINTR since you don't have it on 112? https://codereview.chromium.org/1001673002/diff/230001/util/file/file_io_win.cc File util/file/file_io_win.cc (right): https://codereview.chromium.org/1001673002/diff/230001/util/file/file_io_win.... util/file/file_io_win.cc:85: FILE_SHARE_READ | FILE_SHARE_WRITE, Intentionally using FILE_SHARE_WRITE for LoggingOpenFileForRead?
Message was sent while issue was closed.
https://codereview.chromium.org/1001673002/diff/230001/util/file/file_io_posi... File util/file/file_io_posix.cc (right): https://codereview.chromium.org/1001673002/diff/230001/util/file/file_io_posi... util/file/file_io_posix.cc:106: int rv = HANDLE_EINTR(flock(file, operation)); Robert Sesek wrote: > Drop HANDLE_EINTR since you don't have it on 112? Locking is a blocking operation without LOCK_NB, so EINTR is a possibility.
Message was sent while issue was closed.
https://codereview.chromium.org/1001673002/diff/230001/util/test/thread_win.cc File util/test/thread_win.cc (right): https://codereview.chromium.org/1001673002/diff/230001/util/test/thread_win.c... util/test/thread_win.cc:31: ASSERT_FALSE(platform_thread_); Whoops, should have been ASSERT_TRUE.
Message was sent while issue was closed.
Sorry, didn't mean to rush in before you'd had a chance to review. https://codereview.chromium.org/1001673002/diff/230001/util/file/file_io_posi... File util/file/file_io_posix.cc (right): https://codereview.chromium.org/1001673002/diff/230001/util/file/file_io_posi... util/file/file_io_posix.cc:105: int operation = locking == FileLocking::kShared ? LOCK_SH : LOCK_EX; On 2015/03/20 22:46:55, Robert Sesek wrote: > nit: lost () around the expr Will add that in followup. https://codereview.chromium.org/1001673002/diff/230001/util/file/file_io_posi... util/file/file_io_posix.cc:106: int rv = HANDLE_EINTR(flock(file, operation)); On 2015/03/20 22:46:55, Robert Sesek wrote: > Drop HANDLE_EINTR since you don't have it on 112? I thought it was required here looking at http://linux.die.net/man/2/flock which documents an EINTR error. https://codereview.chromium.org/1001673002/diff/230001/util/file/file_io_win.cc File util/file/file_io_win.cc (right): https://codereview.chromium.org/1001673002/diff/230001/util/file/file_io_win.... util/file/file_io_win.cc:85: FILE_SHARE_READ | FILE_SHARE_WRITE, On 2015/03/20 22:46:55, Robert Sesek wrote: > Intentionally using FILE_SHARE_WRITE for LoggingOpenFileForRead? Yes, that was intentional (it's tested that all combinations succeed now). It sort of got lost in the comments, but ... otherwise the opens will fail with sharing violations, so for our usage it is more helpful if the open succeeds and then the lock subsequently blocks. "Sharing mode" should probably be elevated to an argument to this function in the future.
Message was sent while issue was closed.
Gah, followup here: https://codereview.chromium.org/1022203002/ https://codereview.chromium.org/1001673002/diff/230001/util/test/thread_win.cc File util/test/thread_win.cc (right): https://codereview.chromium.org/1001673002/diff/230001/util/test/thread_win.c... util/test/thread_win.cc:31: ASSERT_FALSE(platform_thread_); On 2015/03/20 23:02:24, Mark Mentovai wrote: > Whoops, should have been ASSERT_TRUE. ! Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
