Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
4721001:
Add multi_process_lock to chrome/common (Closed)
|
Created:
10 years, 1 month ago by dmac Modified:
9 years, 7 months ago CC:
chromium-reviews, Paweł Hajdan Jr., pam+watch_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd multi_process_lock to base
Platform abstraction for a shared lock between processes. The process
that owns the lock will release it on exit even if exit is due
to a crash.
For cloud-print and remoting we want to be able to have a singleton service-process that can run independently of the browser process. This service process will communicate with the browser process via the standard IAC channels, but we want to have a way of signaling to other processes that a) there is a service-process running and b) that it is in a state where it is ready to be communicated with. The multi_process_lock class is intended to work as a simple flag that can be queried from multiple processes. If the service-process should crash, we would like the flag to be cleared automatically so that there is never confusion about the state of the service-process.
Other approaches considered for some Unix/Mac:
- Standard unix domain sockets depend on the file system and don't clean up properly in the case of a crash.
- Shared memory on unix depend on the file system and don't clean up properly in the case of a crash.
- System V semaphores on unix again depend on the file system.
- named_mach_ports on Mac OS. Bootstrap_register_name has been deprecated on 10.6, so we are doing essentially the same thing using CFMessagePort.
On Windows it is implemented as an event.
On Mac it is implemented using a CFMessagePort name.
On Linux it is implement using an abstract name port socket.
TEST=none
BUG=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66323
Patch Set 1 #
Total comments: 61
Patch Set 2 : Fixing up issues. #Patch Set 3 : fixed up some more style issues #Patch Set 4 : get rid of change to base.gypi #Patch Set 5 : Fix up linux compile errors #Patch Set 6 : removed long name test as it tripped a NOTREACHED call. #
Total comments: 12
Patch Set 7 : fix up comments and make recursive #Patch Set 8 : fix up leak of windows handle #Patch Set 9 : forced the over unlock case #
Total comments: 12
Patch Set 10 : Got rid of recursion, fixed up linux bind call #Patch Set 11 : fix up silly linux compile issue #
Total comments: 14
Patch Set 12 : Fixed up based on comments #Patch Set 13 : add some randomization to the test #Patch Set 14 : Clean up missing static value, and clean up stream usage. #Patch Set 15 : fix up unittest on linux #Patch Set 16 : Added missing struct keyword #Patch Set 17 : Fix up missing inttypes.h problem on Windows #Patch Set 18 : Realized that the whole static fix wasn't going to work, and revamped. #Patch Set 19 : Fix up linux not compiling #
Total comments: 26
Patch Set 20 : more clean up for mento #
Total comments: 3
Messages
Total messages: 41 (0 generated)
sanjeev, I'm hoping you will look at the windows side. Evan, I'm hoping you will look at the unix side. Tom, I'm hoping you will look at the mac side. I want to use these for synchronizing between the service process and the browser processe(s).
HTTP 302 Better Linux Reviewer Location: agl@chromium.org
This change needs a significantly expanded motivation in the description before it can be reviewed.
On 2010/11/08 23:14:05, agl wrote: > This change needs a significantly expanded motivation in the description before > it can be reviewed. agl, I have expanded the motivation. Sorry I was so terse the first time. Please let me know if there is a better way of doing what I want to do.
Thanks for the motivation. I'm worried about the name "SharedMutex". It's similar to the ProcessSingleton that we use to stop multiple instances using the same profile directory. Also, we have WaitableEvent which is multi-process on Windows and I wouldn't want folks to miss that because that's the one to use in most cases. I would go for something like ProcessLock, although I don't esp like the name. http://codereview.chromium.org/4721001/diff/1/5 File base/shared_mutex_linux.cc (right): http://codereview.chromium.org/4721001/diff/1/5#newcode11 base/shared_mutex_linux.cc:11: #include "base/logging.h" include "base/eintr_wrapper.h" http://codereview.chromium.org/4721001/diff/1/5#newcode23 base/shared_mutex_linux.cc:23: size_t max_length = sizeof(address.sun_path); I think this bit gets simpler if you just do if (name_.size() + 1 > sizeof(address.sun_path)) http://codereview.chromium.org/4721001/diff/1/5#newcode24 base/shared_mutex_linux.cc:24: if (snprintf(address.sun_path, max_length, "#%s", name_.c_str()) IMPORTANT: do this: memset(address.sun_path, 0, sizeof(address.sun_path)); memcpy(&address.sun_path[1], name_.data(), name_.size()); (the code, as is, silently fails.) http://codereview.chromium.org/4721001/diff/1/5#newcode30 base/shared_mutex_linux.cc:30: address.sun_path[0] = 0; drop this line http://codereview.chromium.org/4721001/diff/1/5#newcode31 base/shared_mutex_linux.cc:31: int socket_fd = socket(PF_UNIX, SOCK_STREAM, 0); AF_UNIX, not PF_UNIX http://codereview.chromium.org/4721001/diff/1/5#newcode38 base/shared_mutex_linux.cc:38: address_length) == 0) { sizeof(address), not address_length http://codereview.chromium.org/4721001/diff/1/5#newcode40 base/shared_mutex_linux.cc:40: } All the other conditionals are failure tests, then suddenly you turn it inside out. I think this should also be a failure test with: NOTREACHED() << ... HANDLE_EINTR(close(socket_fd)); return false; } fd_ = socket_fd; return true; http://codereview.chromium.org/4721001/diff/1/5#newcode45 base/shared_mutex_linux.cc:45: close(fd_); HANDLE_EINTR(close(fd_)); http://codereview.chromium.org/4721001/diff/1/7 File base/shared_mutex_unittest.cc (right): http://codereview.chromium.org/4721001/diff/1/7#newcode13 base/shared_mutex_unittest.cc:13: const char kMutexName[] = "shared_mutex_unittest"; just make it static for a single constant.
Some overall comments: 1. I agree with agl about the name. Especially because mutex means something specific on Windows (CreateMutex is different from CreateEvent). 2. I had originally implemented this as an event (as opposed to a mutex) in service_process_utils as a cheap trick to implement a tri-state mechanism (the three states being NOT_RUNNING, RUNNING_BUT_NOT_READY and RUNNING_AND_READY). Unless we change the interface of this class to implement a tri-state mechanism, using a mutex would be the better option. 3. To keep the interface clean, perhaps the platform specific bits can be hidden inside the .cc files? (either by defining a static factory and having a subclass for each platform or by using an opaque Core embedded class). I don't feel very strongly about this though. http://codereview.chromium.org/4721001/diff/1/4 File base/shared_mutex.h (right): http://codereview.chromium.org/4721001/diff/1/4#newcode39 base/shared_mutex.h:39: HANDLE event_; We should use a ScopedHandle. http://codereview.chromium.org/4721001/diff/1/8 File base/shared_mutex_win.cc (right): http://codereview.chromium.org/4721001/diff/1/8#newcode19 base/shared_mutex_win.cc:19: std::wstring wname = base::SysUTF8ToWide(name_); Please use string16 instead of wstring. http://codereview.chromium.org/4721001/diff/1/8#newcode20 base/shared_mutex_win.cc:20: std::replace(wname.begin(), wname.end(), '\\', '!'); Not sure whether the escaping belongs here or outside this class. This is especially important in might of my next comment. http://codereview.chromium.org/4721001/diff/1/8#newcode21 base/shared_mutex_win.cc:21: wname = L"Global\\" + wname; You should not add the "Global\\". We want the event to be scoped to the current session. If the caller wants global scoping, they can specify that in the name.
+mark as the one that keeps an eye on base for Mac. General comment - how exactly is "name" used on all platforms? the comment about Global on the Win side seems to imply some meaning there. Do we need to do something to make the name part platform neutral and then have each platform tweak the name accordingly so callers aren't doing an ifdef to send in different names? http://codereview.chromium.org/4721001/diff/1/4 File base/shared_mutex.h (right): http://codereview.chromium.org/4721001/diff/1/4#newcode43 base/shared_mutex.h:43: CFMessagePortRef port_; base::mac::ScopedCFTypeRef?
I can comment about the name. It is essentially a unique way of identifying this object (on Windows, this is the name of the named event or mutex). Each platform may have limitations on the allowed characters in the name (for e.g. '\\' is not a valid character in the name on Windows). Regarding my Global comment, here is how named kernel objects are scoped on Windows. By default, they are scoped to the Terminal Server session under which the app is running (you can have multiple parallel sessions when many users are logged in at the same time using Fast User Switching as an example). This is what we want in most of our use cases. Prefixing "Global\\" makes the name global across all sessions. On 2010/11/09 13:19:01, TVL wrote: > +mark as the one that keeps an eye on base for Mac. > > General comment - how exactly is "name" used on all platforms? the comment > about Global on the Win side seems to imply some meaning there. Do we need to do > something to make the name part platform neutral and then have each platform > tweak the name accordingly so callers aren't doing an ifdef to send in different > names? > > http://codereview.chromium.org/4721001/diff/1/4 > File base/shared_mutex.h (right): > > http://codereview.chromium.org/4721001/diff/1/4#newcode43 > base/shared_mutex.h:43: CFMessagePortRef port_; > base::mac::ScopedCFTypeRef?
On 2010/11/08 23:57:49, agl wrote: > Thanks for the motivation. > > I'm worried about the name "SharedMutex". It's similar to the ProcessSingleton > that we use to stop multiple instances using the same profile directory. I was actually considering looking at ProcessSingleton implementation and seeing if it was possible to rewrite it based on this class. I am happy to change the name. Does "SharedLock" work any better? anybody else have any better suggestions? I'll work on cleaning up the code as it stands.
On Tue, Nov 9, 2010 at 12:28 PM, <dmaclach@chromium.org> wrote: > I was actually considering looking at ProcessSingleton implementation > and seeing if it was possible to rewrite it based on this class. I am > happy to change the name I don't believe that ProcessSingleton should be merged into this class. > Does "SharedLock" work any better? anybody else have any better suggestions? > I'll work on cleaning up the code as it stands. I don't like the 'Shared'. It doesn't communicate the right thing to me. "MultiProcessLock", "ProcessLock", are getting closer. AGL
I don’t like SharedMutex or really anything with Shared in the name either. http://codereview.chromium.org/4721001/diff/1/4 File base/shared_mutex.h (right): http://codereview.chromium.org/4721001/diff/1/4#newcode10 base/shared_mutex.h:10: #include <string> System <headers> before project "headers." http://codereview.chromium.org/4721001/diff/1/4#newcode12 base/shared_mutex.h:12: #if OS_MACOSX Nope - #if defined(OS_MACOSX). http://codereview.chromium.org/4721001/diff/1/4#newcode14 base/shared_mutex.h:14: #elif OS_WIN Same. http://codereview.chromium.org/4721001/diff/1/4#newcode28 base/shared_mutex.h:28: SharedMutex(const std::string& name); explicit. http://codereview.chromium.org/4721001/diff/1/4#newcode38 base/shared_mutex.h:38: #if OS_WIN #if defined(…) again. http://codereview.chromium.org/4721001/diff/1/5 File base/shared_mutex_linux.cc (right): http://codereview.chromium.org/4721001/diff/1/5#newcode16 base/shared_mutex_linux.cc:16: } Blank line before, and |} // namespace base|. http://codereview.chromium.org/4721001/diff/1/5#newcode25 base/shared_mutex_linux.cc:25: >= static_cast<int>(max_length)) { I think it’s way more usual for the >= to be at the end of the preceding line. http://codereview.chromium.org/4721001/diff/1/5#newcode26 base/shared_mutex_linux.cc:26: NOTREACHED() << "Socket name to long"; What, now? to? http://codereview.chromium.org/4721001/diff/1/5#newcode33 base/shared_mutex_linux.cc:33: NOTREACHED() << "Couldn't create socket"; For failling sysaclls, you can use [D]PLOG to be sure that the errno is reported. http://codereview.chromium.org/4721001/diff/1/5#newcode45 base/shared_mutex_linux.cc:45: close(fd_); This should be wrapped in if (fd_ != -1) to avoid having close set errno to EBADF on, say, destruction. http://codereview.chromium.org/4721001/diff/1/6 File base/shared_mutex_mac.cc (right): http://codereview.chromium.org/4721001/diff/1/6#newcode6 base/shared_mutex_mac.cc:6: #include "base/mac/scoped_cftyperef.h" Blank line before. http://codereview.chromium.org/4721001/diff/1/6#newcode12 base/shared_mutex_mac.cc:12: } Blank line before, and } // namespace base. http://codereview.chromium.org/4721001/diff/1/6#newcode17 base/shared_mutex_mac.cc:17: port_ = CFMessagePortCreateLocal(NULL, I’d have just thrown all of these on the same line. http://codereview.chromium.org/4721001/diff/1/6#newcode26 base/shared_mutex_mac.cc:26: if (port_) { port_ could have just been a ScopedCFTypeRef. http://codereview.chromium.org/4721001/diff/1/7 File base/shared_mutex_unittest.cc (right): http://codereview.chromium.org/4721001/diff/1/7#newcode13 base/shared_mutex_unittest.cc:13: const char kMutexName[] = "shared_mutex_unittest"; Don’t indent namespace contents. http://codereview.chromium.org/4721001/diff/1/7#newcode14 base/shared_mutex_unittest.cc:14: } } // namespace http://codereview.chromium.org/4721001/diff/1/8 File base/shared_mutex_win.cc (right): http://codereview.chromium.org/4721001/diff/1/8#newcode27 base/shared_mutex_win.cc:27: if (event_) { Don’t we have a ScopedHandle you could use?
To what extent do people think this belongs in base? This seems like kind of a specific concept that would not be appropriate for most applications. Are there other options for where this could live?
I would be OK with this living in chrome/common. On Tue, Nov 9, 2010 at 9:44 AM, <brettw@chromium.org> wrote: > To what extent do people think this belongs in base? This seems like kind > of a > specific concept that would not be appropriate for most applications. Are > there > other options for where this could live? > > > http://codereview.chromium.org/4721001/show >
On Tue, Nov 9, 2010 at 9:28 AM, <dmaclach@chromium.org> wrote: > I was actually considering looking at ProcessSingleton implementation and > seeing > if it was possible to rewrite it based on this class. I am happy to change > the > name. ProcessSingleton locks per user-data-dir: only one instance of Chrome using a given user-data-dir. Your object seems to lock per computer: only one instance of Chrome on a given computer. (Consider network file systems to see the difference.)
Evan, the name that is used is derived from the user_data_dir, so this *can* be (and this *is* how it is used currently) used on a per user-data-dir basis. On Tue, Nov 9, 2010 at 9:52 AM, Evan Martin <evan@chromium.org> wrote: > On Tue, Nov 9, 2010 at 9:28 AM, <dmaclach@chromium.org> wrote: > > I was actually considering looking at ProcessSingleton implementation and > > seeing > > if it was possible to rewrite it based on this class. I am happy to > change > > the > > name. > > ProcessSingleton locks per user-data-dir: only one instance of Chrome > using a given user-data-dir. > > Your object seems to lock per computer: only one instance of Chrome on > a given computer. > > (Consider network file systems to see the difference.) >
In other places in Chrome, we use a named pipe for communication. If the pipe is open, then you know the other side is alive and accepting connections. Can something like this model work for your use case?
On 2010/11/09 19:17:18, brettw wrote: > In other places in Chrome, we use a named pipe for communication. If the pipe is > open, then you know the other side is alive and accepting connections. Can > something like this model work for your use case? brettw, we don't really have generic named pipes on linux/mac. Only on Windows.
On 2010/11/09 17:37:15, agl wrote: > On Tue, Nov 9, 2010 at 12:28 PM, <mailto:dmaclach@chromium.org> wrote: > > I was actually considering looking at ProcessSingleton implementation > > and seeing if it was possible to rewrite it based on this class. I am > > happy to change the name > > I don't believe that ProcessSingleton should be merged into this class. > > > Does "SharedLock" work any better? anybody else have any better suggestions? > > I'll work on cleaning up the code as it stands. > > I don't like the 'Shared'. It doesn't communicate the right thing to > me. "MultiProcessLock", "ProcessLock", are getting closer. > > > AGL Is there anyone unhappy with MultiProcessLock? and by unhappy I mean you would refuse to let the code get checked in if I named it MultiProcessLock? Also, is anybody opposed to having it in chrome/common?
I am OK with MultiProcessLock. I am also OK with the code being in chrome/common. My other review comments still apply :) On Wed, Nov 10, 2010 at 4:16 PM, <dmaclach@chromium.org> wrote: > On 2010/11/09 17:37:15, agl wrote: > > On Tue, Nov 9, 2010 at 12:28 PM, <mailto:dmaclach@chromium.org> wrote: >> > I was actually considering looking at ProcessSingleton implementation >> > and seeing if it was possible to rewrite it based on this class. I am >> > happy to change the name >> > > I don't believe that ProcessSingleton should be merged into this class. >> > > > Does "SharedLock" work any better? anybody else have any better >> suggestions? >> > I'll work on cleaning up the code as it stands. >> > > I don't like the 'Shared'. It doesn't communicate the right thing to >> me. "MultiProcessLock", "ProcessLock", are getting closer. >> > > > AGL >> > > Is there anyone unhappy with MultiProcessLock? and by unhappy I mean you > would > refuse to let the code get checked in if I named it MultiProcessLock? > > Also, is anybody opposed to having it in chrome/common? > > > > > http://codereview.chromium.org/4721001/ >
dmaclach@chromium.org wrote: > Is there anyone unhappy with MultiProcessLock? and by unhappy I mean you > would refuse to let the code get checked in if I named it MultiProcessLock? > > Also, is anybody opposed to having it in chrome/common? No responses in the negative, you’re probably fine. To be more concrete about things, I’m OK with chrome/common/multi_process_lock.h.
Performed rename and cleaned up issues. Running through try bots now. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex.h File base/shared_mutex.h (right): http://codereview.chromium.org/4721001/diff/1/base/shared_mutex.h#newcode10 base/shared_mutex.h:10: #include <string> On 2010/11/09 17:43:14, Mark Mentovai wrote: > System <headers> before project "headers." Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex.h#newcode12 base/shared_mutex.h:12: #if OS_MACOSX On 2010/11/09 17:43:14, Mark Mentovai wrote: > Nope - #if defined(OS_MACOSX). Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex.h#newcode14 base/shared_mutex.h:14: #elif OS_WIN On 2010/11/09 17:43:14, Mark Mentovai wrote: > Same. Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex.h#newcode28 base/shared_mutex.h:28: SharedMutex(const std::string& name); On 2010/11/09 17:43:14, Mark Mentovai wrote: > explicit. Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex.h#newcode38 base/shared_mutex.h:38: #if OS_WIN On 2010/11/09 17:43:14, Mark Mentovai wrote: > #if defined(…) again. Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex.h#newcode39 base/shared_mutex.h:39: HANDLE event_; On 2010/11/09 06:21:05, sanjeevr wrote: > We should use a ScopedHandle. Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex.h#newcode43 base/shared_mutex.h:43: CFMessagePortRef port_; On 2010/11/09 13:19:01, TVL wrote: > base::mac::ScopedCFTypeRef? Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_linux.cc File base/shared_mutex_linux.cc (right): http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_linux.cc#newc... base/shared_mutex_linux.cc:11: #include "base/logging.h" On 2010/11/08 23:57:49, agl wrote: > include "base/eintr_wrapper.h" Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_linux.cc#newc... base/shared_mutex_linux.cc:23: size_t max_length = sizeof(address.sun_path); On 2010/11/08 23:57:49, agl wrote: > I think this bit gets simpler if you just do > if (name_.size() + 1 > sizeof(address.sun_path)) Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_linux.cc#newc... base/shared_mutex_linux.cc:24: if (snprintf(address.sun_path, max_length, "#%s", name_.c_str()) On 2010/11/08 23:57:49, agl wrote: > IMPORTANT: do this: > memset(address.sun_path, 0, sizeof(address.sun_path)); > memcpy(&address.sun_path[1], name_.data(), name_.size()); > > (the code, as is, silently fails.) Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_linux.cc#newc... base/shared_mutex_linux.cc:25: >= static_cast<int>(max_length)) { On 2010/11/09 17:43:14, Mark Mentovai wrote: > I think it’s way more usual for the >= to be at the end of the preceding line. Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_linux.cc#newc... base/shared_mutex_linux.cc:26: NOTREACHED() << "Socket name to long"; On 2010/11/09 17:43:14, Mark Mentovai wrote: > What, now? to? Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_linux.cc#newc... base/shared_mutex_linux.cc:30: address.sun_path[0] = 0; On 2010/11/08 23:57:49, agl wrote: > drop this line Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_linux.cc#newc... base/shared_mutex_linux.cc:31: int socket_fd = socket(PF_UNIX, SOCK_STREAM, 0); On 2010/11/08 23:57:49, agl wrote: > AF_UNIX, not PF_UNIX Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_linux.cc#newc... base/shared_mutex_linux.cc:33: NOTREACHED() << "Couldn't create socket"; On 2010/11/09 17:43:14, Mark Mentovai wrote: > For failling sysaclls, you can use [D]PLOG to be sure that the errno is > reported. Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_linux.cc#newc... base/shared_mutex_linux.cc:38: address_length) == 0) { On 2010/11/08 23:57:49, agl wrote: > sizeof(address), not address_length Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_linux.cc#newc... base/shared_mutex_linux.cc:40: } On 2010/11/08 23:57:49, agl wrote: > All the other conditionals are failure tests, then suddenly you turn it inside > out. I think this should also be a failure test with: > NOTREACHED() << ... > HANDLE_EINTR(close(socket_fd)); > return false; > } > > fd_ = socket_fd; > return true; Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_linux.cc#newc... base/shared_mutex_linux.cc:45: close(fd_); On 2010/11/08 23:57:49, agl wrote: > HANDLE_EINTR(close(fd_)); Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_linux.cc#newc... base/shared_mutex_linux.cc:45: close(fd_); On 2010/11/09 17:43:14, Mark Mentovai wrote: > This should be wrapped in if (fd_ != -1) to avoid having close set errno to > EBADF on, say, destruction. Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_mac.cc File base/shared_mutex_mac.cc (right): http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_mac.cc#newcode6 base/shared_mutex_mac.cc:6: #include "base/mac/scoped_cftyperef.h" On 2010/11/09 17:43:14, Mark Mentovai wrote: > Blank line before. Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_mac.cc#newcode12 base/shared_mutex_mac.cc:12: } On 2010/11/09 17:43:14, Mark Mentovai wrote: > Blank line before, and } // namespace base. Sorry... what do you mean? A couple of us read your comment and couldn't parse it ;-) http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_mac.cc#newcode17 base/shared_mutex_mac.cc:17: port_ = CFMessagePortCreateLocal(NULL, On 2010/11/09 17:43:14, Mark Mentovai wrote: > I’d have just thrown all of these on the same line. Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_mac.cc#newcode26 base/shared_mutex_mac.cc:26: if (port_) { On 2010/11/09 17:43:14, Mark Mentovai wrote: > port_ could have just been a ScopedCFTypeRef. Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_unittest.cc File base/shared_mutex_unittest.cc (right): http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_unittest.cc#n... base/shared_mutex_unittest.cc:13: const char kMutexName[] = "shared_mutex_unittest"; On 2010/11/08 23:57:49, agl wrote: > just make it static for a single constant. Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_unittest.cc#n... base/shared_mutex_unittest.cc:13: const char kMutexName[] = "shared_mutex_unittest"; On 2010/11/09 17:43:14, Mark Mentovai wrote: > Don’t indent namespace contents. Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_unittest.cc#n... base/shared_mutex_unittest.cc:14: } On 2010/11/09 17:43:14, Mark Mentovai wrote: > } // namespace Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_win.cc File base/shared_mutex_win.cc (right): http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_win.cc#newcode19 base/shared_mutex_win.cc:19: std::wstring wname = base::SysUTF8ToWide(name_); On 2010/11/09 06:21:05, sanjeevr wrote: > Please use string16 instead of wstring. Done. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_win.cc#newcode20 base/shared_mutex_win.cc:20: std::replace(wname.begin(), wname.end(), '\\', '!'); On 2010/11/09 06:21:05, sanjeevr wrote: > Not sure whether the escaping belongs here or outside this class. This is > especially important in might of my next comment. Pulled escaping out. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_win.cc#newcode21 base/shared_mutex_win.cc:21: wname = L"Global\\" + wname; On 2010/11/09 06:21:05, sanjeevr wrote: > You should not add the "Global\\". We want the event to be scoped to the current > session. If the caller wants global scoping, they can specify that in the name. removed, and added comment in header about it. http://codereview.chromium.org/4721001/diff/1/base/shared_mutex_win.cc#newcode27 base/shared_mutex_win.cc:27: if (event_) { On 2010/11/09 17:43:14, Mark Mentovai wrote: > Don’t we have a ScopedHandle you could use? Done.
LGTM for Linux http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... File chrome/common/multi_process_lock_linux.cc (right): http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... chrome/common/multi_process_lock_linux.cc:23: // +2 because: 1 for terminator, and 1 for \0 at the front that makes terminator? These aren't NUL terminated names.
Are you planning to change the interface to be tri-state? (not running, running-not-ready and running-ready) http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... File chrome/common/multi_process_lock_win.cc (right): http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... chrome/common/multi_process_lock_win.cc:19: event_.Set(CreateEvent(NULL, FALSE, FALSE, wname.c_str())); CreateEvent will succeed even if the event already existed. You need to check for GetLastError() != ERROR_ALREADY_EXISTS.
http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... File chrome/common/multi_process_lock.h (right): http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... chrome/common/multi_process_lock.h:24: virtual bool TryLock() = 0; Once you write “virtual” you should think long and hard about declaring a virtual destructor. Actually, you shouldn’t have to think too long or too hard, you should just provide one. And in this case, it’s the right thing to do. As it stands now, any likely implementation that would use this code would leak like a sieve. http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... chrome/common/multi_process_lock.h:29: protected: protected sucks. I don’t think you need the constructor or the variable here. I think they properly belong defined individually on each of the concrete derived implementations, as needed. It’s up to those implementations if they even need to keep name_ around, if the constructor will work based on a name parameter or something else (or a name parameter in addition to something else), etc… (If you must, you can declare the MultiProcessLock() constructor as protected and do-nothing, but it doesn’t make much of a practical difference because this class is pure abstract.) http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... chrome/common/multi_process_lock.h:33: }; #include "base/basictypes.h" [...] private: DISALLOW_COPY_AND_ASSIGN(MultiProcessLock); Right? http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... File chrome/common/multi_process_lock_linux.cc (right): http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... chrome/common/multi_process_lock_linux.cc:26: NOTREACHED() << "Socket name too long"; name_, and thus its length, is fully in the caller’s control. This isn’t really NOTREACHED, it’s a normal failure case that you hope to never hit.
I wrote: > http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... > chrome/common/multi_process_lock.h:33: }; > #include "base/basictypes.h" > [...] > private: > DISALLOW_COPY_AND_ASSIGN(MultiProcessLock); > > Right? Wrong! I wrote that before I realized you had “= 0” and had made this class abstract. You don’t need DISALLOW_COPY_AND_ASSIGN here.
Hooray for chrome/common. Thanks for finding away to avoid adding to base.
Also, I’m not sure if you considered this, but: Not being able to allocate objects of the base class kind of sucks if you want to be able to have these locks live on the stack. The workaround is writing scoped_ptr<MultiProcessLock>. It’s kind of icky, but I guess you’ve decided on using the factory model to be more of a purist with the outer interface.
On Thu, Nov 11, 2010 at 2:55 PM, <mark@chromium.org> wrote: > Also, I’m not sure if you considered this, but: > > Not being able to allocate objects of the base class kind of sucks if you > want > to be able to have these locks live on the stack. The workaround is writing > scoped_ptr<MultiProcessLock>. It’s kind of icky, but I guess you’ve decided > on > using the factory model to be more of a purist with the outer interface. > > http://codereview.chromium.org/4721001/ > Yes. I know it's kind of icky, but I did knowingly make that decision. Sorry about missing the virtual d'tor... sigh. Leave C++ for a few days and it bites you in the ass ;-)
On 2010/11/11 22:06:56, sanjeevr wrote: > Are you planning to change the interface to be tri-state? (not running, > running-not-ready and running-ready) No... I can do this through the use of two different locks. Not all platforms give me a good way of doing tri-state.
Also made lock recursive. http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... File chrome/common/multi_process_lock.h (right): http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... chrome/common/multi_process_lock.h:24: virtual bool TryLock() = 0; On 2010/11/11 22:08:27, Mark Mentovai wrote: > Once you write “virtual” you should think long and hard about declaring a > virtual destructor. Actually, you shouldn’t have to think too long or too hard, > you should just provide one. And in this case, it’s the right thing to do. > > As it stands now, any likely implementation that would use this code would leak > like a sieve. Rookie mistake. D'oh. Done. Good catch. http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... chrome/common/multi_process_lock.h:29: protected: I disagree that protected sucks, but hey. I agree with you that name_ should be pushed down into the impls. http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... chrome/common/multi_process_lock.h:33: }; On 2010/11/11 22:08:27, Mark Mentovai wrote: > #include "base/basictypes.h" > [...] > private: > DISALLOW_COPY_AND_ASSIGN(MultiProcessLock); > > Right? Is that intended as "is this correct?" It doesn't hurt, but you are correct that it is unnecessary for a pure virtual class. Done. http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... File chrome/common/multi_process_lock_linux.cc (right): http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... chrome/common/multi_process_lock_linux.cc:23: // +2 because: 1 for terminator, and 1 for \0 at the front that makes On 2010/11/11 21:50:53, agl wrote: > terminator? These aren't NUL terminated names. agl, as far as I know you either have to NUL terminate the name OR pass in an exact length down in bind. How does bind know the length of the string to use as the name if it isn't NUL terminated or given an exact name? I did some research here and I'm startled by the fact that no one seems to be able to agree how bind is to be called in this case. http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... chrome/common/multi_process_lock_linux.cc:26: NOTREACHED() << "Socket name too long"; On 2010/11/11 22:08:27, Mark Mentovai wrote: > name_, and thus its length, is fully in the caller’s control. This isn’t really > NOTREACHED, it’s a normal failure case that you hope to never hit. Changed, and test added. http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... File chrome/common/multi_process_lock_win.cc (right): http://codereview.chromium.org/4721001/diff/33001/chrome/common/multi_process... chrome/common/multi_process_lock_win.cc:19: event_.Set(CreateEvent(NULL, FALSE, FALSE, wname.c_str())); On 2010/11/11 22:06:56, sanjeevr wrote: > CreateEvent will succeed even if the event already existed. You need to check > for GetLastError() != ERROR_ALREADY_EXISTS. Done.
I’m not in love with the proposed design of the recursive stuff considering the duplication. http://codereview.chromium.org/4721001/diff/49001/chrome/common/multi_process... File chrome/common/multi_process_lock_mac.cc (right): http://codereview.chromium.org/4721001/diff/49001/chrome/common/multi_process... chrome/common/multi_process_lock_mac.cc:14: : name_(name), count_(0) { } Will this be accessed from multiple threads? If so (or if this is multithreading-safe on other platforms and you want the Mac to match), fix up. If not, use preincrement/decrement when you operate on count, because I think that’s what folks are used to seeing. http://codereview.chromium.org/4721001/diff/49001/chrome/common/multi_process... chrome/common/multi_process_lock_mac.cc:15: OK, I just looked at your implementation on other platforms and see that it’s the same deal. Maybe you’d be better off putting all of that “count” logic in some outer-outer platform-dependent class, like RecursiveMultiProcessLock, that wraps MultiProcessLock? http://codereview.chromium.org/4721001/diff/49001/chrome/common/multi_process... chrome/common/multi_process_lock_mac.cc:40: DLOG(ERROR) << "Over unlocked MultiProcessLock " << name_; I’d hyphenate this dude. http://codereview.chromium.org/4721001/diff/49001/chrome/common/multi_process... chrome/common/multi_process_lock_mac.cc:52: base::mac::ScopedCFTypeRef<CFMessagePortRef> port_; Improve the struct packing. Put port_ before count_. http://codereview.chromium.org/4721001/diff/49001/chrome/common/multi_process... File chrome/common/multi_process_lock_win.cc (right): http://codereview.chromium.org/4721001/diff/49001/chrome/common/multi_process... chrome/common/multi_process_lock_win.cc:43: DLOG(ERROR) << "Over unlocked MultiProcessLock " << name_; hypheny (and the Linux file too.) http://codereview.chromium.org/4721001/diff/49001/chrome/common/multi_process... chrome/common/multi_process_lock_win.cc:54: int count_; packy
http://codereview.chromium.org/4721001/diff/49001/chrome/common/multi_process... File chrome/common/multi_process_lock_mac.cc (right): http://codereview.chromium.org/4721001/diff/49001/chrome/common/multi_process... chrome/common/multi_process_lock_mac.cc:14: : name_(name), count_(0) { } On 2010/11/11 23:45:24, Mark Mentovai wrote: > Will this be accessed from multiple threads? > > If so (or if this is multithreading-safe on other platforms and you want the Mac > to match), fix up. > > If not, use preincrement/decrement when you operate on count, because I think > that’s what folks are used to seeing. Good point regarding multi threads. I got rid of the recursion as I don't need it currently, and there's no point in over designing right now. If somebody needs it later they can do the wrapper class as you suggested. http://codereview.chromium.org/4721001/diff/49001/chrome/common/multi_process... chrome/common/multi_process_lock_mac.cc:15: On 2010/11/11 23:45:24, Mark Mentovai wrote: > OK, I just looked at your implementation on other platforms and see that it’s > the same deal. > > Maybe you’d be better off putting all of that “count” logic in some outer-outer > platform-dependent class, like RecursiveMultiProcessLock, that wraps > MultiProcessLock? Done. http://codereview.chromium.org/4721001/diff/49001/chrome/common/multi_process... chrome/common/multi_process_lock_mac.cc:40: DLOG(ERROR) << "Over unlocked MultiProcessLock " << name_; On 2010/11/11 23:45:24, Mark Mentovai wrote: > I’d hyphenate this dude. Done. http://codereview.chromium.org/4721001/diff/49001/chrome/common/multi_process... chrome/common/multi_process_lock_mac.cc:52: base::mac::ScopedCFTypeRef<CFMessagePortRef> port_; On 2010/11/11 23:45:24, Mark Mentovai wrote: > Improve the struct packing. Put port_ before count_. Done. http://codereview.chromium.org/4721001/diff/49001/chrome/common/multi_process... File chrome/common/multi_process_lock_win.cc (right): http://codereview.chromium.org/4721001/diff/49001/chrome/common/multi_process... chrome/common/multi_process_lock_win.cc:43: DLOG(ERROR) << "Over unlocked MultiProcessLock " << name_; On 2010/11/11 23:45:24, Mark Mentovai wrote: > hypheny > (and the Linux file too.) Done. http://codereview.chromium.org/4721001/diff/49001/chrome/common/multi_process... chrome/common/multi_process_lock_win.cc:54: int count_; On 2010/11/11 23:45:24, Mark Mentovai wrote: > packy Done.
LGTM for the Windows code with a nit. http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... File chrome/common/multi_process_lock_win.cc (right): http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... chrome/common/multi_process_lock_win.cc:27: HANDLE event = CreateEvent(NULL, FALSE, FALSE, wname.c_str()); Nit: You don't need to use a HANDLE here. You could use a ScopedHandle or your could just the event_ member variable itself and just call reset() if it already exists. Will make the code more readable.
Mac LGTM. Design LGTM. Test questions, and I’ll defer to Adam and Sanjeev for Linux and Windows. http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... File chrome/common/multi_process_lock_mac.cc (right): http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... chrome/common/multi_process_lock_mac.cc:34: DLOG(ERROR) << "Over unlocked MultiProcessLock - " << name_; When I said “hyphenate,” I meant “over-unlocked”. http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... File chrome/common/multi_process_lock_unittest.cc (right): http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:11: static const char kMutexName[] = "shared_mutex_unittest"; Should you randomize this name a little bit so that a hung test or two tests running simultaneously or other badness won’t cause flake? http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:25: "we have a path that is to long for linux to handle."; Should the interface mandate a specific upper limit on all platforms for consistency? http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:52: // Will cause LOG in debug, but will complete. Indent? Put a blank line before this so that it’s obvious that it applies to the second TryLock. http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:55: // Will cause LOG in debug, but will complete. Same. http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:59: You should have some sort of test that proves that a lock is released when it goes out of scope. This would have caught your “oops, virtual destructor” bug (and someone’s future similar bug.)
Sorry for all the revisions. http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... File chrome/common/multi_process_lock_mac.cc (right): http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... chrome/common/multi_process_lock_mac.cc:34: DLOG(ERROR) << "Over unlocked MultiProcessLock - " << name_; On 2010/11/12 17:58:28, Mark Mentovai wrote: > When I said “hyphenate,” I meant “over-unlocked”. Done. http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... File chrome/common/multi_process_lock_unittest.cc (right): http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:11: static const char kMutexName[] = "shared_mutex_unittest"; On 2010/11/12 17:58:28, Mark Mentovai wrote: > Should you randomize this name a little bit so that a hung test or two tests > running simultaneously or other badness won’t cause flake? Done. http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:25: "we have a path that is to long for linux to handle."; On 2010/11/12 17:58:28, Mark Mentovai wrote: > Should the interface mandate a specific upper limit on all platforms for > consistency? Done. http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:52: // Will cause LOG in debug, but will complete. On 2010/11/12 17:58:28, Mark Mentovai wrote: > Indent? > > Put a blank line before this so that it’s obvious that it applies to the second > TryLock. Done. http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:55: // Will cause LOG in debug, but will complete. On 2010/11/12 17:58:28, Mark Mentovai wrote: > Same. Done. http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:59: On 2010/11/12 17:58:28, Mark Mentovai wrote: > You should have some sort of test that proves that a lock is released when it > goes out of scope. This would have caught your “oops, virtual destructor” bug > (and someone’s future similar bug.) Done. http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... File chrome/common/multi_process_lock_win.cc (right): http://codereview.chromium.org/4721001/diff/49002/chrome/common/multi_process... chrome/common/multi_process_lock_win.cc:27: HANDLE event = CreateEvent(NULL, FALSE, FALSE, wname.c_str()); On 2010/11/12 17:47:56, sanjeevr wrote: > Nit: You don't need to use a HANDLE here. You could use a ScopedHandle or your > could just the event_ member variable itself and just call reset() if it already > exists. Will make the code more readable. Fixed
This is pretty good. I think these are the only things that remain. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... File chrome/common/multi_process_lock.h (right): http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock.h:17: // The length of a multi process lock name is limited on Linux, so Multiprocess is spelled without a space, but you can use a hyphen if you like, which would better match multi_process_lock and MultiProcessLock. Same on line 23. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock.h:19: // not include a terminator. This is defined in terms of UNIX_PATH_MAX-2 on You should have a COMPILE_ASSERT to guarantee this in _linux.cc. In the COMPILE_ASSERT, you should probably use sizeof(sun_path) instead of UNIX_PATH_MAX as the latter is nonstandard. And spaces go around operators like - even when they’re in comments. :) http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock.h:21: static const size_t MULTI_PROCESS_LOCK_NAME_MAX_LEN = 106; You need to #include <sys/types.h> before you can use size_t. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... File chrome/common/multi_process_lock_linux.cc (right): http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_linux.cc:37: memset(&address, 0, sizeof(address)); Or you could have just written struct sockaddr_un address = {0}; http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... File chrome/common/multi_process_lock_unittest.cc (right): http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:18: static char kMutexName[] = "shared_mutex_unittest " __DATE__ " " __TIME__; This is no good. Multiple invocations of the same binary will still sit on top of one another. This isn’t a good candidate for use of __DATE__ and __TIME__. Throwing some runtime randomness at the problem would be fine. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:35: "have a path that is to long for linux to handle " __DATE__ " " __TIME__); And these guys are just gratuitous. Take them out. (My __DATE__ and __TIME__ vendetta partially stems from the fact that they’re sort of ambiguous in light of rebuilds and systems that cache object files that result from compilation.) http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:69: // Check to see that lock is released when it goes out of scope. This doesn’t really check with {scoping}, which is kind of what I was hoping for. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:73: EXPECT_TRUE(test_lock->TryLock()); Also, this will still be true if the lock is still held from the preceding TryLock (if the Unlock doesn’t happen for some reason, like a destructor not marked virtual). This doesn’t make a very thorough test. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:78: TEST_F(MultiProcessLockTest, MaxNameLength) { Ah, I see what you’ve done. I think the COMPILE_ASSERT in the Linux implementation file is better, though. (See base/logging.h.) http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:96: Extra blank line.
http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... File chrome/common/multi_process_lock_linux.cc (right): http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_linux.cc:37: memset(&address, 0, sizeof(address)); On 2010/11/15 23:42:55, Mark Mentovai wrote: > Or you could have just written > struct sockaddr_un address = {0}; I usually go with the memset just in case someone compiles with pedantic on. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... File chrome/common/multi_process_lock_unittest.cc (right): http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:18: static char kMutexName[] = "shared_mutex_unittest " __DATE__ " " __TIME__; On 2010/11/15 23:42:55, Mark Mentovai wrote: > This is no good. Multiple invocations of the same binary will still sit on top > of one another. This isn’t a good candidate for use of __DATE__ and __TIME__. > Throwing some runtime randomness at the problem would be fine. How do you suggest adding the randomness you seek? Remember that the subprocesses that are launched need to have the same mutex name so that they can look them up. I looked into using pids to do this and found that we don't have code to get the parent pid on windows. I thought this was a reasonable compromise. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:69: // Check to see that lock is released when it goes out of scope. On 2010/11/15 23:42:55, Mark Mentovai wrote: > This doesn’t really check with {scoping}, which is kind of what I was hoping > for. Why isn't the scoped_ptr with a reset not giving us a scoping check? http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:73: EXPECT_TRUE(test_lock->TryLock()); On 2010/11/15 23:42:55, Mark Mentovai wrote: > Also, this will still be true if the lock is still held from the preceding > TryLock (if the Unlock doesn’t happen for some reason, like a destructor not > marked virtual). This doesn’t make a very thorough test. Good point. I'll look into this one... http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:78: TEST_F(MultiProcessLockTest, MaxNameLength) { On 2010/11/15 23:42:55, Mark Mentovai wrote: > Ah, I see what you’ve done. > > I think the COMPILE_ASSERT in the Linux implementation file is better, though. > > (See base/logging.h.) Ah... missed the assert. Went looking for it, and didn't find it.
http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... File chrome/common/multi_process_lock_unittest.cc (right): http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:18: static char kMutexName[] = "shared_mutex_unittest " __DATE__ " " __TIME__; Can you pass a cookie to the processes on the command line? Or use the environment? http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:69: // Check to see that lock is released when it goes out of scope. > Why isn't the scoped_ptr with a reset not giving us a scoping check? It simulates it, but I was hoping for a check that actually worked with crossing a scope boundary, because I envision that as a possible use case for this code.
OK, added some getenv/setenv functions. Will move those over to process_utils in a separate change. There are a couple other places in the code that can use them. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... File chrome/common/multi_process_lock.h (right): http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock.h:17: // The length of a multi process lock name is limited on Linux, so On 2010/11/15 23:42:55, Mark Mentovai wrote: > Multiprocess is spelled without a space, but you can use a hyphen if you like, > which would better match multi_process_lock and MultiProcessLock. Same on line > 23. Done. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock.h:19: // not include a terminator. This is defined in terms of UNIX_PATH_MAX-2 on On 2010/11/15 23:42:55, Mark Mentovai wrote: > You should have a COMPILE_ASSERT to guarantee this in _linux.cc. > > In the COMPILE_ASSERT, you should probably use sizeof(sun_path) instead of > UNIX_PATH_MAX as the latter is nonstandard. > > And spaces go around operators like - even when they’re in comments. :) Done. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock.h:21: static const size_t MULTI_PROCESS_LOCK_NAME_MAX_LEN = 106; On 2010/11/15 23:42:55, Mark Mentovai wrote: > You need to #include <sys/types.h> before you can use size_t. Didn't seem to be needed on any platform, but added it anyways. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... File chrome/common/multi_process_lock_unittest.cc (right): http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:18: static char kMutexName[] = "shared_mutex_unittest " __DATE__ " " __TIME__; On 2010/11/15 23:42:55, Mark Mentovai wrote: > This is no good. Multiple invocations of the same binary will still sit on top > of one another. This isn’t a good candidate for use of __DATE__ and __TIME__. > Throwing some runtime randomness at the problem would be fine. Done. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:35: "have a path that is to long for linux to handle " __DATE__ " " __TIME__); On 2010/11/15 23:42:55, Mark Mentovai wrote: > And these guys are just gratuitous. Take them out. > > (My __DATE__ and __TIME__ vendetta partially stems from the fact that they’re > sort of ambiguous in light of rebuilds and systems that cache object files that > result from compilation.) Done. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:69: // Check to see that lock is released when it goes out of scope. On 2010/11/15 23:42:55, Mark Mentovai wrote: > This doesn’t really check with {scoping}, which is kind of what I was hoping > for. Done. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:73: EXPECT_TRUE(test_lock->TryLock()); On 2010/11/15 23:42:55, Mark Mentovai wrote: > Also, this will still be true if the lock is still held from the preceding > TryLock (if the Unlock doesn’t happen for some reason, like a destructor not > marked virtual). This doesn’t make a very thorough test. Done. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:78: TEST_F(MultiProcessLockTest, MaxNameLength) { On 2010/11/15 23:42:55, Mark Mentovai wrote: > Ah, I see what you’ve done. > > I think the COMPILE_ASSERT in the Linux implementation file is better, though. > > (See base/logging.h.) Done. http://codereview.chromium.org/4721001/diff/75001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:96: On 2010/11/15 23:42:55, Mark Mentovai wrote: > Extra blank line. Done.
LGTM! http://codereview.chromium.org/4721001/diff/84001/chrome/common/multi_process... File chrome/common/multi_process_lock.h (right): http://codereview.chromium.org/4721001/diff/84001/chrome/common/multi_process... chrome/common/multi_process_lock.h:9: #include <string> C++ headers should follow C headers. <string> after <sys/types.h>. http://codereview.chromium.org/4721001/diff/84001/chrome/common/multi_process... File chrome/common/multi_process_lock_unittest.cc (right): http://codereview.chromium.org/4721001/diff/84001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:52: bool MultiProcessLockTest::setenv(const std::string &name, Does base/environmenth.h help here?
http://codereview.chromium.org/4721001/diff/84001/chrome/common/multi_process... File chrome/common/multi_process_lock_unittest.cc (right): http://codereview.chromium.org/4721001/diff/84001/chrome/common/multi_process... chrome/common/multi_process_lock_unittest.cc:186: // are perusing thge build logs. drive-by typo: thge |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
