Community
Participate
Working Groups
The profile registry currently has a mechanism to lock a profile, but it is only an in-memory one. In circumstances where 2 instances could be running of the same profile / profile registry we need to have a mechanism that prevents concurrent modification of the same profile. For that we should consider locking the profile on disk in the profile registry. For the actual locking implementation see the Locker classes in org.eclipse.osgi. Things to think about: - Make sure that the loaded profile is always the most recent one - Is there any interaction with the shared profile - Does / should the GC lock the profile? What happens when a profile is locked?
Here is one strategy I am considering 1. You acquire an exclusive lock before modifying a profile. In other words, if you are just reading the profile no locking is needed. 2. Another instance will not let the user modify the profile if it failed to acquire an exclusive lock. 3. The in-memory profile is validated against the on-disk contents before it is to be modified.
this sounds good.
Created attachment 120739 [details] Code plugin patch I have attached an initial implementation of the locking mechanism. Pretty localized in the SimpleProfileRegistry class.
Created attachment 120741 [details] Tests plugin patch I have added a test in the ProfileRegistryTest class to test this. I faked the cross process locking by creating a lock file explicitly.
Thanks Pradeep. It looks fine for a Profile 1.1 approach however I'm wondering if you could look at adding an NIO approach that will "really" do the locking if nio is available. I'm more or less convinced that in the cross-process cases where we really care about locking we'll have nio available. I'm thinking some logic similar to BasicLocation.createLocker to choose the locker type.
I would really encourage you to reuse the Locker classes (maybe moving them to a better place) out of osgi rather than copying. I'm sure Tom will not see any reason to move this class around to allow for its reuse if we were annoyed by the dependency on an impl package. Also I think we could take the factory method to another class if necessary.
Thanks Simon and Pascal, for your quick feedback. I totally agree with both the comments. Not at all a fan of code duplicity! In fact I did think of re-using it but figured I should first fix it locally, add a test case and then venture out to refactor and re-use. That's why I said "initial" implementation. The purpose of this is to make it clear how this will blend-in with the current code. I propose we move the Locker to org.eclipse.osgi.util or a its own new package org.eclipse.osgi.locks, provide a factory (as Pascal suggested) in lieu of BasicLocation#createLocker(). Then move the two locker implementations to <locker-package>.internal.impl. Thoughts or suggestions?
Tom, any thought on where you would want to see the Locker related classes to be moved?
Can you simply use the createLocation method to create the location you want to lock? org.eclipse.osgi.service.datalocation.Location.createLocation(Location, URL, boolean) You should be able to get one of any of the Location services registered and call this method to create your own location to lock on. This hides all the logic of picking nio when it is available.
Yes Tom, I am able to do this. It works almost the way I wanted. But there is a small problem. The concrete impl of Location that I am getting is BasicLocation. This creates the lock file as ".metadata/.lock" inside the profile dir and Location does not provide a way to choose my own Locker (or lock file). I would prefer to create the lock file right inside the profile registry itself, as <profile-name>.lock, for easy discovery. Moreover, if we don't anticipate creating a ".metadata" folder inside each profile dir, then it seems silly to do this just for the sake of a lock file. Either we need a way to choose a context-specific lock file or we need to agree that <profile-dir>/.metadata/.lock is the way to go. Let us choose the right lock file from the beginning itself so that we can avoid messy code to handle backward compatibilities later on. (1) Spruce-up Location to allow a Locker (another set() variant). Of course the Locker implementations should also be extensible (eg: choosing lock file). OR (2) Let us choose <profile-dir>/.metadata/.lock as the lock file. Which way to go?
Created attachment 121286 [details] Code plugin patch - v2 This code patch incorporates the suggestions from Simon, Pascal and Tom. It is a complete re-write of the implementation. This assumes the lock file to be .metadata/.lock as in (2) of Comment #10. Note that the code change in SimpleProfileRegistry#removeProfile(profileId) is not part of this feature implementation, but a fix for a bug that was uncovered by external locking of profiles.
Created attachment 121288 [details] Tests plugin patch - v2 Added a test case to ProfileRegistryTest to test the profile locking mechanism. This simulates external locking by way of a tiny console app - SimpleFileLockerApp in package org.eclipse.equinox.p2.tests.engine, which is invoked as an external process via SimpleFileLockerApp.jar kept in dir /org.eclipse.equinox.p2.tests/testData/engineTest. The next attachment has the jar file.
Created attachment 121289 [details] Java app used by test This is the jar file that ProfileRegistryTest#testProfileLocking() uses to kick off another process for simulating external locking. This jar should be controlled in the dir /org.eclipse.equinox.p2.tests/testData/engineTest. This should be committed along with the tests plugin patch.
The changes are with respect to the source in Head as of Dec 28, 2pm GMT. Except for an import, SimpleProfileRegistry isn't different from the source base in M4 (Dec 17). BTW, even if we need to choose a different lock file, I would prefer to do it in another pass (in M5 itself) just to keep the changes under control.
(In reply to comment #10) > Yes Tom, I am able to do this. It works almost the way I wanted. But there > is a small problem. > The concrete impl of Location that I am getting is BasicLocation. This > creates the lock file as ".metadata/.lock" inside the profile dir and Location > does not provide a way to choose my own Locker (or lock file). I would prefer > to create the lock file right inside the profile registry itself, as > <profile-name>.lock, for easy discovery. Moreover, if we don't anticipate > creating a ".metadata" folder inside each profile dir, then it seems silly to > do this just for the sake of a lock file. > Either we need a way to choose a context-specific lock file or we need to > agree that <profile-dir>/.metadata/.lock is the way to go. Let us choose the > right lock file from the beginning itself so that we can avoid messy code to > handle backward compatibilities later on. > (1) Spruce-up Location to allow a Locker (another set() variant). Of course the > Locker implementations should also be extensible (eg: choosing lock file). > OR If we decide this is the way to go then we could add another set variant that took a lock file name: org.eclipse.osgi.service.datalocation.Location.set(URL, boolean, String) Where the last String parameter specifies the lock file name to use. If a relative path is used then the file lock would be contained within the location URL specified. If absolute then we would use the absolute path for the lock file. Would that solve your needs? > (2) Let us choose <profile-dir>/.metadata/.lock as the lock file. > > Which way to go? > Another option is to use the StorageManager API. This is used by various components in Eclipse to manage multiple versions of files used by multiple instances of an Eclipse process. For example, the Framework and the extension registry use this API to manage the files under configuration/org.eclipse.osgi and configuration/org.eclipse.core.runtime directories respectively. Older instances of the files are kept alive as long as the older instances are running. Is there a need to support instance versions of the files in a profile? and clean up the old versions when the old instances get shutdown? If so then you should probably look into using StorageManager.
(In reply to comment #15) > If we decide this is the way to go then we could add another set variant that > took a lock file name: > > org.eclipse.osgi.service.datalocation.Location.set(URL, boolean, String) > > Where the last String parameter specifies the lock file name to use. If a > relative path is used then the file lock would be contained within the location > URL specified. If absolute then we would use the absolute path for the lock > file. Would that solve your needs? Yes, this should help. > Is there a need to support instance versions of the files in a profile? and > clean up the old versions when the old instances get shutdown? If so then you > should probably look into using StorageManager. No. It is an exclusive lock and so there should only be one instance holding the lock at any given time.
(In reply to comment #16) > Yes, this should help. Do you think you could provide a patch for this? Contributions are welcome here also ;-)
(In reply to comment #17) > > Do you think you could provide a patch for this? Contributions are welcome > here also ;-) Sure Tom. It makes sense as I have the use-case ready.
Created attachment 121407 [details] Location API addition patch Patch for plug-ins (1) org.eclipse.osgi Added a new Location#set() method that takes lock file name/path as an arg. Implemented this in BasicLocation. (2) org.eclipse.osgi.tests Added three new tests to BasicLocationTests.
Created attachment 121408 [details] Code plugin patch - v3 Changed the code in SimpleProfileRegistry to use the new API in Location (implemented by the patch attached above). Now the lock file is ".lock" directly inside the profile directory.
Created attachment 121409 [details] Tests plugin patch - v3 Changed ProfileRegistryTest to react to the change in profile lock file as mentioned in the previous comment/patch.
Created attachment 121410 [details] Java app used by test - v2 Recreated the Java app jar used by SimpleProfileLock#testProfileLocking(). Please use this version to commit to org.eclipse.equinox.p2.tests/testData/engineTest.
I was looking at integrating this stuff into HEAD however I think there are still a few things we need to cover. The locking in the patch covers updating and removing of profiles but not retrieval. I think we're going to need to have some sort of check in getProfileMap to cover the case where a profile has been modified externally. As it stands now we won't ever go back to disk to check for an updated version. The other area that needs thinking is that the location lock/unlock is a trylock and the current behaviour in Lock is a full wait lock. I haven't dug deeply here but one possiblity is to use a layered locking mechanism where the process lock is a try lock and the thread lock is a regular wait lock. In this setup we wouldn't release the process lock until all pending lock requests in the process are handled.
(In reply to comment #23) > I was looking at integrating this stuff into HEAD however I think there are > still a few things we need to cover. Thanks for looking into this, Simon. > > The locking in the patch covers updating and removing of profiles but not > retrieval. I think we're going to need to have some sort of check in > getProfileMap to cover the case where a profile has been modified externally. > As it stands now we won't ever go back to disk to check for an updated version. I didn't change the locking points; just extended the locking mechanism. Yes, if we need to ensure the integrity of the returned profiles, we should validate all profiles during getProfileMap() or for that matter during getProfile(id) or getProfiles(). But I have two concerns: 1. It would be too heavy-weight for a "getter" to do an I/O always. 2. It still does not guarantee integrity because the profile could be modified after it has been returned. If we need to do this let us do that on another bug as it is orthogonal to cross profile locking. As it stands currently, there are checks in place to ensure that a profile cannot be updated or removed if the contents on the disk has changed. Another approach would be to provide an API "isStale()" on IProfile so that clients has a way of verifying it - even profile itself could do it before allowing certain operations on it. > > The other area that needs thinking is that the location lock/unlock is a > trylock and the current behaviour in Lock is a full wait lock. I haven't dug > deeply here but one possiblity is to use a layered locking mechanism where the > process lock is a try lock and the thread lock is a regular wait lock. In this > setup we wouldn't release the process lock until all pending lock requests in > the process are handled. Good point. It is already meant to be a layered approach - ie, a different thread from the same process will block while a different process will trylock. While at it, I just realized that the unlock wasn't waiting for all nested locks to unwind before releasing the process lock. I have fixed that right now. If there aren't any other changes, I shall attach a new code patch (for SimpleProfileRegistry) with this change.
> I didn't change the locking points; just extended the locking mechanism. Indeed. We're unfortunately treating the whole thing as a cache and subsequently one of the hard things to do is to handle updates that don't go through a notification mechanism in the cache. > Yes, if we need to ensure the integrity of the returned profiles, we should > validate all profiles during getProfileMap() or for that matter during > getProfile(id) or getProfiles(). But I have two concerns: > 1. It would be too heavy-weight for a "getter" to do an I/O always. > 2. It still does not guarantee integrity because the profile could be modified > after it has been returned. These are really good concerns and where I'm at too. To be clear I am not paricularly concerned about the up-to-dateness of the profile when reading (concern 2) as this can always happen. At least for the in process case we're protecting against the more dangerous situation where we're performing a transaction on an out-of-date profile. I'm less sure that we're doing a good job in handling out-of-process modifications. I suspect we need some mechanism to flush the cache at appropriate points. Off the top of my head it seems reasonable to flush the cache when doing a write operation and recognizing that the current cache doesn't match what's on disk. More challenging given your very reasonable I/O concern is the need to (perhaps occasionally) verify what's on disk is consistent with the cache. Maybe something like an expiry on the call to getProfiles as I would tend to avoid a background thread. I don't think this problem is orthogonal and should be addressed in this bug before closing it. > While at it, I just realized that the unlock wasn't waiting for all nested > locks to unwind before releasing the process lock. I have fixed that right now. > If there aren't any other changes, I shall attach a new code patch (for > SimpleProfileRegistry) with this change. That would be great.
(In reply to comment #19) > Created an attachment (id=121407) [details] > Location API addition patch > > Patch for plug-ins > (1) org.eclipse.osgi > Added a new Location#set() method that takes lock file name/path as > an arg. > Implemented this in BasicLocation. > (2) org.eclipse.osgi.tests > Added three new tests to BasicLocationTests. > I am reviewing this patch now. Simon has a concern about the semantics of Location.lock() (which we chatted about offline). The Location.lock method has tryLock semantics, it does not block if the lock cannot be acquired successfully. We could consider adding a lock(timeout) method that would try to acquire the lock and timeout after a specified time if the lock is not successful instead of returning immediately. Right now I do not see a requirement to add this method to the Location API. In my view it is just a convenience method that can be implemented by clients of Location by calling the lock() method for a particular period of time (with some sleeps in between so you don't spin the CPU :) This is a separate issue from this bug which currently only needs to have the ability to pick the Location lock file. I propose we release the OSGi patch for Location this week to get the API changes in sooner. We can work out the other issues in p2 code after the OSGi patch has been released. Thoughts?
+1 for going ahead with pradeep's location patch. With the lock layering discussed earlier the trylock semantics are going to be what we want anyway.
Created attachment 121843 [details] Code plugin patch - v4 Patch for org.eclipse.p2.engine. This fixes nested unlock issue that I noticed when confirming the layered locking implementation Simon suggested.
Created attachment 121844 [details] Tests plugin patch - v4 Patch for org.eclipse.equinox.p2.tests. Added an additional test for testing nested lock/unlock scenarios which would have caught the issue I ran into while code inspection. Note that this patch also requires the Java app Jar attached here to be put in the dir testData/engineTest.
Tom (in response to Comment #26) - Thanks for reviewing the location API patch. Simon (in response to Comment #25) - Now am I with you. It wasn't clear to me that your concern about stale cache is in the context of multiple processes. We have a guard already in place to prevent a stale copy from overwriting a newer content - SimpleProfileRegistry#lockProfile() already checks the time stamp before proceeding. Thus as long as we lock the profile (via this API) prior to any modification, we are safe. But we do not have any checks in place to figure out whether we are "reading" from a stale copy or not. One option is to hoist up the above mentioned lockProfile() as an API so that paranoid clients could call it before they operate on it (even if it is for reading purposes). Ideally it should be a "push" than a "poll" to figure out the staleness. That is, the Profile should be able to listen to "profile store events" to know it is out-dated. For example, if "profile store" is Windows filesystem, it generates events when filesystem content changes.
(In reply to comment #19) > Created an attachment (id=121407) [details] > Location API addition patch > > Patch for plug-ins > (1) org.eclipse.osgi > Added a new Location#set() method that takes lock file name/path as > an arg. > Implemented this in BasicLocation. > (2) org.eclipse.osgi.tests > Added three new tests to BasicLocationTests. > I released this patch with some javadoc modifications. Please review.
Thanks Pradeep. Chatting with Pascal we talked about doing the minimal thing. In that spirit probably the simplest mechanism for handling stale caches is with a "refresh" method on the profile registry. This gets us out of the business of trying to guess an an appropriate expire time and puts the onus on the client. We still likely want to cause a refresh when a write detects we're out of synch although conceivably we might have the user refresh the profile registry in that case. Taking a look at your patch now as I think we have all the necessary bits and pieces.
So, what's next?
In addition to the cross process checks that you have, could you please add a few test verifying the proper behavior of the in-process locking. Thx.
(In reply to comment #32) > Thanks Pradeep. > > Chatting with Pascal we talked about doing the minimal thing. In that spirit > probably the simplest mechanism for handling stale caches is with a "refresh" > method on the profile registry. This gets us out of the business of trying to > guess an an appropriate expire time and puts the onus on the client. > > We still likely want to cause a refresh when a write detects we're out of synch > although conceivably we might have the user refresh the profile registry in > that case. Do you want me to look at providing a refresh() API? > Taking a look at your patch now as I think we have all the necessary bits and > pieces. Great! Thanks Simon!
(In reply to comment #34) > In addition to the cross process checks that you have, could you please add a > few test verifying the proper behavior of the in-process locking. Thx. > Yes Pascal, yesterday I realized there aren't any for in-process locking. So I added one for same thread access (it is there in the tests patch I uploaded yesterday). But there isn't one yet for multi-threaded in-process access. I could look into adding one for this scenario too.
In reply to comment #33) > So, what's next? I'm still working on integrating Pradeep's patch but will be adding it in shortly. (In reply to comment #35) > Do you want me to look at providing a refresh() API? I've got something simple for this in my workspace.
(In reply to comment #36) > (In reply to comment #34) > > In addition to the cross process checks that you have, could you please add a > > few test verifying the proper behavior of the in-process locking. Thx. > > > > Yes Pascal, yesterday I realized there aren't any for in-process locking. So I > added one for same thread access (it is there in the tests patch I uploaded > yesterday). But there isn't one yet for multi-threaded in-process access. I > could look into adding one for this scenario too. > This comment made me look to see if we have an Location tests that actually test attempts to lock a Location multiple times from the same VM instance. We do not :( While developing a testcase to test this I found bug 260416.
Thanks Pradeep, I've committed all of your test bundles and a modified version of your changes in SimpleProfileRegistry. The main differences are: 1) ProfileLock class - I separated this out and in the process merged the few remaining relevant pieces of the original Lock class. 2) number of tweaks on locking and unlocking to take into consideration timestamps of what we have on disk 3) additional locking when we populate the profile cache 4) a resetProfiles method to allow resetting of the profile cache. In the context of this bug this is currently only being used when we encounter an out of date profile when performing a write operation. Although I think the functionality of this bug is now in good shape and I want to see it integrated in a nightly build I'm going to leave it open as I'd like to verify our test code coverage (eclemma) and where necessary add a few more tests to maintain close to 100% coverage.
Glad to hear that. Thanks Simon! Sure, you can keep the bug open until it is ready. I have accepted all the changes. In the meanwhile I will look at adding a test for in-process but multi-threaded locking (may take a couple of days or more) I am currently tied up with another set of activities.
(In reply to comment #39) > The main differences are: > 1) ProfileLock class - I separated this out and in the process merged the few > remaining relevant pieces of the original Lock class. Simon - I just noticed a semantic change that was done as part of this merging wherein the thread-lock is no longer blocking. That is, when another thread requests a lock it returns false immediately if it is already locked. (See comments #23, #24 for related discussions).
Created attachment 122924 [details] Tests plugin patch - v5: Added a new test Pascal, I have added a new test in ProfileRegistryTest for in-process multi-threaded access scenario based on the current locking semantics in ProfileLock.
Thanks Pradeep. You're right about the change in semantics and ultimately we went back to the original semantics -- see bug 261573. I took your new tests as a starting point and tweaked them to do what we now want and have committed them. Marking FIXED.
Thanks Simon. Even though I had accepted your changes that day, failed to notice the semantic change made until I went about writing the test (in fact I assumed it blocked). Glad that all is well again.
