I agree with the earlier argument about not larding startup with things like writing new files to id the coming-up Chrome to late-coming instances. An alternative might be to acquire a lock to protect the profile, and write an id asynchronously after startup. The late-coming instance would see the lock and wait for the id to appear (or the lock to evaporate). This does mean the late-coming instance will have to wait sometimes, but in those cases the coming-up instance presumably is not ready to respond to requests anyhow. On Mac and Linux, all we need for the lock is an agreed-upon open file descriptor, which should be easy to find. We could acquire the lock in parallel with other startup activity which doesn't modify system state.
-scott On Tue, May 5, 2009 at 10:16 AM, Amanda Walker <[email protected]> wrote: > > Thanks for elaborating. > > So if we look at it another way, there are several different problems > that ProcessSingleton is currently used to solve: > > - Prevent multiple instances of Chromium from referring to the same > profile/user data directory. In the general sense, what we need here > is a way to acquire and test locks on data directories. This is a > cross-platform requirement: all platforms should be able to launch > instances of Chromium in different user sessions with different user > data / profiles (in order to support fast user switching on Mac & > Windows, single Linux boxes with multiple people logged in, etc. > Currently, ProcessSingleton really only solves this by accident. It > seems cleaner to me to meet this requirement with an explicit > mechanism: Something like a "ProfileLock" or "DirectoryLock," for > example. This also gives a way to ease this requirement if someone > fixes the concurrent-access problem, without affecting other uses of > ProcessSingleton. > > - Testing, since a number of tests are not multiple-instance safe. I > would describe this as a defect in those tests or the test framework, > which again ProcessSingleton only mitigates by accident. I would > prefer we address this by fixing the tests and/or test scripts so that > we can indeed run tests in parallel without worrying about them > stepping on each other. We have a strong desire to reduce our build > cycle time, of which having to run tests serially instead of in > parallel is a large component. > > - Within a user session, ensuring that requests to open URLs get > processed by a currently running instance of Chromium instead of > launching a new one. This is where platform differences are > strongest, and is ProcessSingleton's main function. Registering > properties on windows or the like for Windows and X11 seems cleaner to > me than putting breadcrumbs into the file system, since window > properties are inherently tied to the user session and to an active > instance of the application (and thus do not need to be cleaned up on > a crash, etc.). On the Mac, this function is provided by the OS, so > there's no need for Chromium to duplicate it. > > --Amanda > > > On Tue, May 5, 2009 at 12:27 PM, Никита Офицеров <[email protected]> wrote: >> >> I'm sorry for the late response, I've been quite busy last week. >> >> 2009/4/27 Amanda Walker <[email protected]>: >>> "Hacky" is fairly subjective: are there particular things about the >>> existing implementation that bother you? Currently they are different >>> for each OS because each OS has its own issues surrounding launching >>> multiple instances of the browser, different mechanisms for handling >>> incoming requests from the OS to open web pages, and so on. >>> >> >> 2009/4/28 stoyan <[email protected]>: >>> Well, on Windows code looks racy, does not work well across multiple >>> desktops, and probably will create troubles accessing userdata when >>> same instance is running in different terminal sessions (with same >>> user account). >> >> The problem with quick, baroque source code is that usually there are >> lots of corner cases. >> I have rethought the idea after your comments, and there are several >> problems with current >> Windows implementation: >> >> 1. As Stoyan mentioned, window messaging does not work across multiple >> desktops. >> Therefore it doesn't work across multiple terminal sessions. >> >> Consider the following use case: A user has a version of Windows that >> allows multiple >> consequent terminal sessions on his PC, and leaves Chrome opened. Then >> he opens a >> session from his laptop and launches Chrome. Preferred result is >> opening a new window >> in the second session, but AFAIK it's impossible for the Chrome master >> process to open a >> window in a different window station. Hence quite a lot of changes are >> required for this to >> function, like delegating handling of a widget to the newly created >> process, and these fall >> out of the scope of this idea. But current ProcessSingleton will just >> fail to detect another >> Chrome instance and crash or show weird behavior. If it worked, we >> could at least show some >> message telling user that running Chrome from different terminal >> sessions is not yet implemented. >> So IMHO ProcessSingleton should not rely on window messages, that are >> by definition >> bound to one window station. >> >> 2. Another problem with Windows implementation (maybe not really a >> problem) is that it >> guards Chrome against being launched with the same (luckily, >> case-insensitive) user data >> dir path. This is a fundamental problem, because ProcessSingleton >> should protect not only >> against being launched with the same path, but against actually using >> the same dir twice >> (one directory can be addressed with different paths, because of links >> and mounting, even on >> Windows NTFS partitions). >> Well, I think that without user intervention a situation that exploits >> this cannot be created, so >> it may be considered as ok, but it's still a problem that IMHO should be >> fixed. >> >> Because of that, I suggest two ideas: >> 1. Do almost as I suggested before, but instead of Chrome PID use a >> GUID, and write it only once, >> on the first run. This GUID will let us distinguish data dirs without >> need to rely on their path. So at >> normal launch it will be one additional disk read (just several bytes) >> and some IPC. If you think that >> we can't afford even one extra disk read, then think about my second idea: >> >> 2. If problem #2 is not really a problem, then instead of GUID or PID >> we can just use some hash from >> user data dir path as channel_id, and make no disk operations at all. >> This is by no means worse than >> current implementation and solves the first problem. However, this is >> only a partial solution. >> >> As an additional benefit, ProcessSingleton becomes what it was meant >> to be - a facility to ensure that >> only one instance with given data dir is running. Opening new URLs and >> getting browser PID will be >> just IPC messages, without custom protocol parsing or some extra >> quirks in ProcessSingleton code. >> >> 2009/4/27 Amanda Walker <[email protected]>: >>> Simply eliminating differences between per-platform implementations is >>> not necessarily a large benefit by itself, since the amount of code is >>> small and concerns an area where the three platforms behave quite >>> differently. >> I do agree with this, but it's not only for eliminating differences >> between implementations, >> but also to improve them. There is already quite a lot of >> platform-specific code in IPC, >> and it can handle differences quite well. The only remaining thing is >> disabling this functionality >> completely on Mac when not running tests, and this can be handled by one >> 'if'. >> >> As for "racyness", this can also be improved. Current implementation >> was racy because singleton >> was created with some delay after checking that no Chrome process >> existed. That was a bug, and >> that was fixed by adding singleton creation in constructor. It isn't >> actually atomic, but should work >> well. Creating named pipe is atomic operation now >> (FILE_FLAG_FIRST_PIPE_INSTANCE), >> however, creating UNIX domain socket is not. So this may also be >> improved in order to avoid >> race conditions. >> >> Nikita Ofitserov >> >> > >> > > > > --~--~---------~--~----~------------~-------~--~----~ Chromium Developers mailing list: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~----------~----~----~----~------~----~------~--~---
