Bugzilla – Full Text Bug Listing
| Summary: | Objects are GC'ed out of a view even when adapters are attached | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Tom Crockett <Thomas.M.Crockett> | ||||||||
| Component: | cdo.core | Assignee: | Simon Mc Duff <smcduff> | ||||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||||
| Severity: | enhancement | ||||||||||
| Priority: | P1 | CC: | smcduff, stepper | ||||||||
| Version: | 2.0 | Flags: | stepper:
galileo+
stepper: review- |
||||||||
| Target Milestone: | M4 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Mac OS X - Carbon (unsup.) | ||||||||||
| Whiteboard: | Lighter, Faster and Better | ||||||||||
| Bug Depends on: | 257270 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
What if you kept around strong references to the adapters that had been added to an object, and if the object was GC'ed and then reloaded you would silently re-add its adapter? (In reply to comment #1) > What if you kept around strong references to the adapters that had been added > to an object, and if the object was GC'ed and then reloaded you would silently > re-add its adapter? > If your adapter matches CDOChangeSubscriptionPolicy, I know it will add it to a strong reference hashmap. But it will also registered it to the server. It is probably not want you are looking for. From the newsgroup:
This is not hard to do, and it's how I've solved the problem, I'm just concerned that it is a very big "gotcha" for anyone new to CDO. It took me a full day of debugging to figure out what the heck was going on and why objects kept losing their adapters... it turned out it wasn't that the adapters were disappearing but that the objects were disappearing, but that was not easy to figure out! I think if well-documented it's fine to leave as-is though.
Eike Stepper wrote:
> Could it be better to leave it to the adapter implementor to meet this decision and take care to strongly reference the adapter from somewhere (and the object from the adapter)?
After discussion, it make senses. We will add the following: - A policy to determine which adapter you should keep in memory. - Infrastructure to keep in memory objects that are important. As an end-user, you will have to choose which policy you want to use. Is that work for you ? Simon Sounds great, I'd love to see this happen! Created attachment 119456 [details]
Patch v1
- Changed name CDOChangeSubscriptionPolicy to CDOAdapterPolicy
- Changed view.setChangeSubscriptionPolicy to view.setAdapterDeltaNotificationPolicy
- view.setAdapterReferencePolicy()
- Public interface CommitCOntext
- CHANGE - CDOTransactionHandler.committingTransaction(CDOTransaction, CommitCOntext)
- NEW CDOTransactionHandler.committedTransaction(CDOTransaction, CommitCOntext)
- Testcases
- If you like how I introduce ViewAdapterManager, I will make the same changes for CDOChangeSubscriptionManager.
- By default objects are GC's Should it be CDOAdapterPolicy.ALL ?
If you agree with these name change I will update the javadoc as well before committing.
I'm not sure if I like the following change
> - Changed view.setChangeSubscriptionPolicy to
> view.setAdapterDeltaNotificationPolicy
Let me know if you prefer the old method name or if you find a better name that could link SetAdapterReferencePolicy and setChangeSubscriptionPolicy.
Simon
(In reply to comment #7) > I'm not sure if I like the following change > > > - Changed view.setChangeSubscriptionPolicy to > > view.setAdapterDeltaNotificationPolicy > > Let me know if you prefer the old method name or if you find a better name that > could link SetAdapterReferencePolicy and setChangeSubscriptionPolicy. I like the name setChangeSubscriptionPolicy(CDOAdapterPolicy ). What about setStrongReferencePolicy(CDOAdapterPolicy ) for the new functionality? (In reply to comment #8) > (In reply to comment #7) > > I'm not sure if I like the following change > > > > > - Changed view.setChangeSubscriptionPolicy to > > > view.setAdapterDeltaNotificationPolicy > > > > Let me know if you prefer the old method name or if you find a better name that > > could link SetAdapterReferencePolicy and setChangeSubscriptionPolicy. > > I like the name setChangeSubscriptionPolicy(CDOAdapterPolicy > ). > > What about setStrongReferencePolicy(CDOAdapterPolicy > ) for the new functionality? > the method name doesn't show anything to say it is for adapter.. (only the parameter) ??? Created attachment 119621 [details]
Patch v2
Created attachment 119692 [details]
Patch v3
I've removed the CDOTransactionHandler interface from TransactionAdapterManager because the only method is called directly anyway. Now it becomes a bit questionable if ViewAdapterManager and TransactionAdapterManager are needed or if some methods in CDOViewImpl plus one in CDOTransactionImpl are enough.
Some time I said that I'd like to see an umbrella concept for all the possible changes in a transaction. Your new interface CDOCommitContext looks nearly like such. Should we factor out all methods except getTransaction() into CDOChangeDescription? Then CDOCommitContext would be:
public interface CDOCommitContext extends CDOChangeDescription
{
public CDOTransaction getTransaction();
}
REJECTING because 2 test cases in AdapterManagerTest are failing!
(In reply to comment #11) > Created an attachment (id=119692) [details] > Patch v3 > > I've removed the CDOTransactionHandler interface from TransactionAdapterManager > because the only method is called directly anyway. Now it becomes a bit > questionable if ViewAdapterManager and TransactionAdapterManager are needed or > if some methods in CDOViewImpl plus one in CDOTransactionImpl are enough. > > Some time I said that I'd like to see an umbrella concept for all the possible > changes in a transaction. Your new interface CDOCommitContext looks nearly like > such. Should we factor out all methods except getTransaction() into > CDOChangeDescription? Then CDOCommitContext would be: CDOCommitContext is very old.. it was only moved from internal to public.. CDOChangeDescription doesn't exist yet!!! (Where is it ?) When we will create CDOCHangeDescription it should be factor with CDOCOmmitContext. > > public interface CDOCommitContext extends CDOChangeDescription > { > public CDOTransaction getTransaction(); > } > > REJECTING because 2 test cases in AdapterManagerTest are failing! > It is failing.. because of the setReferenceCAche(). I noticed that yesterday... The default now is at SOft... by putting WEAK in the tescases it worked! Simon > CDOCommitContext is very old.. it was only moved from internal to public.. > CDOChangeDescription doesn't exist yet!!! (Where is it ?) When we will create > CDOCHangeDescription it should be factor with CDOCOmmitContext. I mean: Create a new interface CDOChangeDescription and move all methods (except getTransaction()) from CDOCommitContext to CDOChangeDescription. Let CDOCommitContext extend CDOChangeDescription. Maybe we can use that in invalidation events, too... > > public interface CDOCommitContext extends CDOChangeDescription > > { > > public CDOTransaction getTransaction(); > > } > > > > REJECTING because 2 test cases in AdapterManagerTest are failing! > > > It is failing.. because of the setReferenceCAche(). I noticed that yesterday... > The default now is at SOft... by putting WEAK in the tescases it worked! Great, then go ahead ;-) (In reply to comment #11) > Created an attachment (id=119692) [details] > Patch v3 > I've removed the CDOTransactionHandler interface from TransactionAdapterManager > because the only method is called directly anyway. Now it becomes a bit > questionable if ViewAdapterManager and TransactionAdapterManager are needed or > if some methods in CDOViewImpl plus one in CDOTransactionImpl are enough. > Some time I said that I'd like to see an umbrella concept for all the possible > changes in a transaction. Your new interface CDOCommitContext looks nearly like > such. Should we factor out all methods except getTransaction() into > CDOChangeDescription? Then CDOCommitContext would be: > public interface CDOCommitContext extends CDOChangeDescription > { > public CDOTransaction getTransaction(); > } > REJECTING because 2 test cases in AdapterManagerTest are failing! I removed TransactionAdapterManager Use the same startegy for CHangeSubcriptionManager (e.g.: CHangeSubcriptionManager.committedTransaction()); It could be a good idea to add it into the INvalidationEvent. I created a new bugzilla for it : 257803: [POLISH] Refactor Invalidation event to include CDOChangeDescription https://bugs.eclipse.org/bugs/show_bug.cgi?id=257803 I will go ahead with this bugs. Committed to HEAD The default policy for setStrongReference(CDOAdapterPolicy.ALL) :-) Fix available in CDO 2.0.0M4 Generally available. |
Copied from the newsgroup: Eike, It seems to me that adding an adapter to an object that is in an open view is implicitly saying, "I am interested in this object. Please keep me notified of when it changes." Given that CDOViews are "partial views on the repository", it makes sense that things would get GC'ed out of them when no one is paying attention to them, but if I attach an adapter to an object it seems to me that I'm expressing interest in it, and therefore it should be maintained as part of the view. What if objects with adapters attached to them were maintained in a separate Strong reference map? Eike Stepper wrote: Tom, I see it as one of the hidden but main advantages of CDO that the EObjects can be silently discarded from memory when it runs low and automatically recreated when they are needed again. And you're right, I didn't think about existing adapters. Can you please open an enhancement request and make a proposal on how exactly you'd like to have it configurable? Cheers /Eike Tom Crockett schrieb: I've been fighting a very insidious bug for the last 3 days in which adapters I attached to my shared model were mysteriously disappearing. I couldn't find any place where they were getting removed, so I was really mystified for a long time. Today I finally figured out that it was because of the fact that CDOViewImpl stores its objects in a weak ReferenceValueMap, so they were getting GC'ed, along with their adapters! I worked around this fairly easily by attaching an EContentAdapter when first loading a resource which recursively maintains strong references to every object in the view, but I just wanted to mention this for anyone who may be experiencing such problems. Also, I wanted to ask whether this behavior might be made configurable, if it already hasn't, for CDO 2.0. More information: