Bug 266941 - GC trigger should only run when the uninstall and commit occurred in the same profile
Summary: GC trigger should only run when the uninstall and commit occurred in the same...
Status: CLOSED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: p2 (show other bugs)
Version: 3.5   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.6 M7   Edit
Assignee: Pascal Rapicault CLA
QA Contact:
URL:
Whiteboard:
Keywords: polish
Depends on:
Blocks:
 
Reported: 2009-03-03 16:57 EST by John Arthorne CLA
Modified: 2010-04-09 20:43 EDT (History)
1 user (show)

See Also:

Attachments
GCActivator patch (3.63 KB, patch)
2009-04-16 13:29 EDT, Matthew Piggott CLA 		no flags Details | Diff
GCActivator patch (3.63 KB, patch)
2009-04-16 13:47 EDT, Matthew Piggott CLA 		no flags Details | Diff
GCActivator patch (3.65 KB, patch)
2009-04-16 13:48 EDT, Matthew Piggott CLA 		no flags Details | Diff
GCActivator patch (4.51 KB, patch)
2009-04-16 15:50 EDT, Matthew Piggott CLA 		no flags Details | Diff
GCActiavtor patch (2.66 KB, patch)
2009-04-22 11:32 EDT, Matthew Piggott CLA 		no flags Details | Diff
Update the patch to match what is in HEAD (3.21 KB, patch)
2010-04-08 21:46 EDT, Pascal Rapicault CLA 		no flags Details | Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description John Arthorne CLA 2009-03-03 16:57:27 EST 
I20090224

The current gc trigger policy (GCActivator.registerGCTrigger), causes a gc to start if there is an uninstall event followed by a commit event. However, it is possible for two engine sessions to be occurring at concurrently, so it should check that the uninstall and the commit occurred against the same profile, since it is possible for multiple engine sessions to be running concurrently in different profiles. GCing at the wrong time can have fatal results on the profile (see bug 266881).
Comment 1 John Arthorne CLA 2009-03-03 17:02:23 EST 
Note, ideally it should ensure that the uninstall and commit occurred in the same engine session, but the engine events don't currently expose enough information to implement that. Perhaps the event could carry a session id that allows us to check for that.
Comment 2 Matthew Piggott CLA 2009-04-16 13:29:21 EDT 
Created attachment 132107 [details]
GCActivator patch

I spoke with Simon yesterday and this is what we came up with.  The patch should ensure that the GC is only called if a commit results in IUs being removed from a profile.

Is there a good method to test GC behaviour?
Comment 3 Matthew Piggott CLA 2009-04-16 13:47:13 EDT 
Created attachment 132111 [details]
GCActivator patch

Added an additional check for null.
Comment 4 Matthew Piggott CLA 2009-04-16 13:48:31 EDT 
Created attachment 132112 [details]
GCActivator patch

Evidently Eclipse hadn't finished creating the patch!
Comment 5 Matthew Piggott CLA 2009-04-16 15:50:51 EDT 
Created attachment 132130 [details]
GCActivator patch

Missing manifest file
Comment 6 Pascal Rapicault CLA 2009-04-16 21:45:51 EDT 
The patch is trying to do too much in that it should not try to detect if IUs have changed, etc. It should simply keep track of the profile ids (profile objects are too large to hold on to).
Comment 7 Matthew Piggott CLA 2009-04-22 11:32:45 EDT 
Created attachment 132793 [details]
GCActiavtor patch

I've tried to make the change smaller in this version.  

When an uninstall event occurs the profile's ID is stored, when a rollback or commit event occurs the event's profile is compared to the stored value and changes are only made if they match.

If an uninstall event occurs when a profile id is already set only the newer value is kept.  While there is no guarantee, its more likely that the older event will finish before the newer uninstall event so it seems preferable to GC after the uninstall commits.
Comment 8 Pascal Rapicault CLA 2009-04-29 15:17:05 EDT 
The patch looks fine, but this is not crucial, so I would rather wait for another release rather than putting that in and risking late breakage.
Comment 9 Pascal Rapicault CLA 2010-04-08 21:46:58 EDT 
Created attachment 164327 [details]
Update the patch to match what is in HEAD
Comment 10 Pascal Rapicault CLA 2010-04-09 20:43:30 EDT 
I have released a fix for this in HEAD.