Bugzilla – Full Text Bug Listing
| Summary: | [RegionDigraph] add a way to set the default install region | ||
|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Borislav Kapukaranov <b.kapukaranov> |
| Component: | Components | Assignee: | equinox.components-inbox <equinox.components-inbox> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | glyn.normington, katya.stoycheva, tjwatson |
| Version: | unspecified | ||
| Target Milestone: | Juno M3 | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| Whiteboard: | |||
| Attachments: | |||
I think this is the wrong usage of the current thread local and could result in unpredictable results if there are nested install operations. I actually think this may have uncovered an issue with the current thread local. Imagine there is a sync bundle listener (or another bundle EventHook) that reacts to the INSTALLED event and causes the installation of another bundle. This would result in what I am referring to as a nested install operation. If the Region.installBundle method was called on Region X to start the "initial" install then the theadLocal variable will be set to the Region X. The Bundle will be assigned to the Region X in the RegionBundleEventHook.event method. But some other BundleEventHook could have decided to install another bundle to another region Y before the RegionBundleEventHook.event method could be called. This would result in the thread local being set to region Y (and eventually unset to null by completion of install). Now we finally call RegionBundleEventHook.event for the first bundle and the thread local is unset. There is another problematic scenario if the other "nested" BundleEventHook uses a normal BundleContext to install. In this case we would expect the newly installed bundle to be placed in the same region as the calling BundleContext's bundle. But in this case the bundle would be placed in Region X because that is the value of the thread local. We could probably look at ways to fix this, but I am hesitant to expose this internal thread local detail in the API because we would have to think of all the possible ways it could adversely effect the behavior of the automatic assignment of Regions to bundles. Should we instead think of adding API (to RegionDigraph?) that allows someone to set the default Region to use if BundleContext.install method is called? Sorry for the late reply, I was on holiday. I fully agree that these scenarios are problematic. If I'm right the hooks are notified synchroniously. Because I'm not entirely sure how this all works here's a question: In that scenario with two installing hooks 1 - RegionBundleEventHook and 2 - MyNestedInstallingEventHook, after Region.installBundle is called isn't 1) eligible for notification also after the nested install and thus called twice (the second time would be the normal call once the nested operations are gone) and wouldn't that lead again to calling 2)? The case of 2) directly calling BC.install is trickier because we don't have control and knowledge of the digraph. If Region.installBundle was always called it could provide a solution in thread context class loader style (store, set, revert) and go to the next hook. I can't think of any nice solution here, do you have any ideas? Exposing this will probably complicate everything even further, you're right. The RegionDigraph default region option sounds reasonable - if present use it, otherwise revert to the current handling. This should work in our scenarios. (In reply to comment #2) > Sorry for the late reply, I was on holiday. > > I fully agree that these scenarios are problematic. > If I'm right the hooks are notified synchroniously. Because I'm not entirely > sure how this all works here's a question: > In that scenario with two installing hooks > 1 - RegionBundleEventHook and > 2 - MyNestedInstallingEventHook, > after Region.installBundle is called isn't 1) eligible for notification also > after the nested install and thus called twice (the second time would be the > normal call once the nested operations are gone) and wouldn't that lead again > to calling 2)? Yes, but keep in mind we would call these recursively the 2nd invocation since the notification is synchronous. So the thread local would get overwritten by the nested call to Region.install and unset by the nested call to the RegionBundleEventHook. > > The case of 2) directly calling BC.install is trickier because we don't have > control and knowledge of the digraph. If Region.installBundle was always called > it could provide a solution in thread context class loader style (store, set, > revert) and go to the next hook. I can't think of any nice solution here, do > you have any ideas? The solution could involve a more complicated thread local value. Currently we only store the installing Region. Instead we may be able to use a Map<String, Region> that maps a location->region where the location is the reserved bundle location string which is being used to install the bundle and the region is the region into which the bundle is being installed. Then we can lookup the region by location in the RegionBundleEventHook. You could still imagine some nested install operation that would try to install the same bundle location, but in this case I think the framework would prohibit the nested operation because the location string would have already been reserved by the original install operation. For now I am not too worried about fixing this issue since I think we would be dealing with some pretty strange hooks. But if you can think of a useful scenario where a bundle event hook would want to install bundles during synchronous calls then perhaps we should look more into fixing this. > > Exposing this will probably complicate everything even further, you're right. > The RegionDigraph default region option sounds reasonable - if present use it, > otherwise revert to the current handling. This should work in our scenarios. I also don't want to expose the thread local because it is an implementation detail for how we currently determine the region a bundle being installed should belong to. If we expose this impl detail then we may not have the flexibility to change that impl detail in the future. For example, we may not be able to do something like I suggest above easily. Created attachment 202408 [details]
new version with Digraph API
Here's a new version with API added to RegionDigraph.
I've created also a unit test(also attached). I've tested this in the p2 integrated Virgo env and it also works well there.
Tom, is this close to what you had in mind about the default region API?
Created attachment 202409 [details]
testcase for the default region patch
(In reply to comment #4) > Tom, is this close to what you had in mind about the default region API? Yes this is close to what I had in mind. I would also add a test to RegionSystemTests, something like this: public void testDefaultRegion() throws BundleException { digraph.setDefaultAssignRegion(null); Region systemRegion = digraph.getRegion(regionBundle); Region pp1Region = digraph.createRegion(PP1); Bundle pp1Bundle = bundleInstaller.installBundle(PP1, null); Region result = digraph.getRegion(pp1Bundle); assertEquals("Wrong region", systemRegion, result); pp1Bundle.uninstall(); digraph.setDefaultAssignRegion(pp1Region); pp1Bundle = bundleInstaller.installBundle(PP1, null); result = digraph.getRegion(pp1Bundle); assertEquals("Wrong region", pp1Region, result); digraph.setDefaultAssignRegion(null); } Comments: 1) Is there a better name for setDefaultAssignRegion? I think simply setDefaultRegion should be fine. 2) What should happen if the default region is set to a region which is not contained in this digraph? throw an IllegalArgumentException? 3) What should happen if the default region is removed from the digraph? Should that result in the default region being automatically set to null? 4) I assume this is a non persistent setting? If you persist and reload the digraph is the default reset to null? If not we also need to change the persistence code for digraph to include the default region id. Boris, did you see my comment 6 and comment 7? (I know bugzilla e-mail was close to the time I made these comments). (In reply to comment #8) > Boris, did you see my comment 6 and comment 7? (I know bugzilla e-mail was > close to the time I made these comments). I meant to say I know bugzilla e-mail was NOT WORKING close ... Sorry I was again on vacation until today. Thanks for outlining these scenarios - I'll update the patch later today. >3) What should happen if the default region is removed from the digraph? >Should that result in the default region being automatically set to null? Yes, it should. >4) I assume this is a non persistent setting? If you persist and reload the >digraph is the default reset to null? If not we also need to change the >persistence code for digraph to include the default region id. I was thinking about non persistent setting too. Since its usage doesn't expect an already set default region, every user should have it set(when required) before the user action in order to guarantee it's the needed value. Targeting for M3. Note that M2 warmup build is today, so we can put this off until after next week. Ultimately I want you (Boris) to commit the fix to exercise your commit rights. Created attachment 203140 [details]
First iteration of the new default region Digraph API
The first patch for the default region API
Created attachment 203141 [details]
Test cases for the first iteration
Created attachment 203142 [details]
Improved default region API
The improved default region API with enhanced behaviour for removing default region and setting not-existing region as default
Created attachment 203143 [details]
Test cases for the improved default region API
Adds 3 new test cases to the Region System test. One proposed by Tom and two test cases verifying the correct behaviour when removing default region and when trying to set removed region as default.
The patches should be applied in the upload order. (In reply to comment #11) > Ultimately I want you (Boris) to commit the fix to exercise your commit rights. I'd like to try that too :) I'm having a difficult time applying the patches. Are these against the current state of master? (In reply to comment #17) > I'm having a difficult time applying the patches. Are these against the > current state of master? Yes. I just applied them all starting with 0001 to 0004 using cygwin on a clean branch. What's the error? I can try to make a single patch if that will be easier for you? Cygwin's default is to make a separate patch for each step back in the history so it was easier for me this way, but it shouldn't be too hard to prepare a single patch for all changes. BTW I'd highly recommend using the git console. From my experience with Egit (about 7 months) it's the least convenient way to apply a patch... (In reply to comment #19) > BTW I'd highly recommend using the git console. From my experience with Egit > (about 7 months) it's the least convenient way to apply a patch... yes, that is what I am using. I tried both the apply and the am commands but it fails trying to apply the second patch: rt.equinox.bundles watson$ git am < /develop/defects/eclipse/354655/latest/0001-bug354655-Added-default-assign-region-to-the-RegionD.patch Applying: bug354655 - Added default assign region to the RegionDigraph API /develop/equinox/git/rt.equinox.bundles/.git/rebase-apply/patch:116: trailing whitespace. * /develop/equinox/git/rt.equinox.bundles/.git/rebase-apply/patch:124: trailing whitespace. * warning: 2 lines add whitespace errors. The first patch applies but has whitespace errors which I typically have ignored. But then I apply the second patch and I get this: rt.equinox.bundles watson$ git am < /develop/defects/eclipse/354655/latest/0002-bug354655-added-test-case-for-default-assign-region.patch Applying: bug354655 - added test case for default assign region error: patch failed: bundles/org.eclipse.equinox.region.tests/src/org/eclipse/equinox/internal/region/hook/RegionBundleEventHookTests.java:82 error: bundles/org.eclipse.equinox.region.tests/src/org/eclipse/equinox/internal/region/hook/RegionBundleEventHookTests.java: patch does not apply Patch failed at 0001 bug354655 - added test case for default assign region When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". The patch seems to be behind master and does not look to be based on the commit from : http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=620f7deeb1be737dca403e2ae3c84d7a8ebe5752 Created attachment 203170 [details]
all test cases in one patch against latest master
Hm it seems something went wrong with the pull I made from master. It's OK now and here's the new patch that replaces the other test case patches.
You should be able to safely apply 0001, 0003 and the one I just uploaded.
Patches look good. Unfortunately I would like you to release this next week since we are in the middle of building M2 this week. Is that OK with you, or do you need this before then? No problem at all, thanks. I'll ping the bug next week to check if the M2 is OK. I assume M2 is out already so is it OK to try a push? (In reply to comment #24) > I assume M2 is out already so is it OK to try a push? Yes, please push your fix Boris. Thanks! Pushed to master. Closing the bug. Thanks. FYI, the fix for this bug introduced unsafe thread access to the regions Set. While Glyn and I were reviewing thread safety issues in regions (bug 361905) we noticed this. Glyn released a fix with commit: http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=d744d90b5565ac676fcccf3d5fdd35b7f057b3f4 |
Created attachment 201419 [details] Patch with public interface to modifiy the thread local Currently the thread local is modified internally in the Region objects. That effectively means that no external installer can plug in and install in a region-aware manner without being modified to understand regions. The attached patch contains a proposal to illustrate the idea. In the Virgo integration with p2 I'd like to have public way to set the thread local, so that I can modify it before calling simple configurator's apply configuration and ensure my bundles will go where I want and not in the SC's region. I've tested the patch for these purposes and it works nice. wdyt?