Bug 362232

Comment 1 John Ross 2011-11-01 11:04:05 EDT

The new requirement is that all orphaned coordinations must be detected, not just implicit coordinations on the thread local stack.

So the use of WeakReference will be extended to all coordinations. Each CoordinationImpl will be wrapped by a CoordinationReferent that implements Coordination and serves as a proxy. Only the CoordinationReferent is made available externally and is, therefore, the only object that can be strongly referenced by clients. A CoordinationWeakReference (extending WeakReference) is constructed using CoordinationReferent, ReferenceQueue, and CoordinationImpl. Internally, only the CoordinationWeakReference maintains a strong reference to the CoordinationImpl; everything else references CoordinationWeakReference. When a Reference is enqueued, it is cast to a CoordinationWeakReference from which the CoordinationImpl may be obtained. The CoordinationImpl is then failed and ended.

In terms of monitoring the ReferenceQueue, it seems that we'll need a background thread. Currently, the ReferenceQueue is monitored each time an operation is performed on the WeakCoordinationStack, which is only for implicit coordinations. I think the background thread is a better option than introducing monitoring each time a method is invoked on the Coordinator?

Comment 2 BJ Hargrave 2011-11-01 11:20:52 EDT

(In reply to comment #1)
> In terms of monitoring the ReferenceQueue, it seems that we'll need a
> background thread. Currently, the ReferenceQueue is monitored each time an
> operation is performed on the WeakCoordinationStack, which is only for implicit
> coordinations. I think the background thread is a better option than
> introducing monitoring each time a method is invoked on the Coordinator?

You don't necessarily need a thread. Another option is to process the ReferenceQueue for work whenever someone calls into the implementations. For example, every entry point (e.g. Coordination.begin) can call a processReferenceQueue method. So this is basically cycle stealing from client threads. It means there may be a delay in cleaning up orphaned Coordinations, but then we only know they were orphaned when the GC told us so. So a further delay is no big deal. I think I would go this route for now.

Comment 3 BJ Hargrave 2011-11-01 11:22:44 EDT

(In reply to comment #2)
> Another option is to process the
> ReferenceQueue for work whenever someone calls into the implementations.

I think you will also need to process the queue on BundleActivator.stop and ServiceFactory.ungetService. That is, when the impl bundle stops and whenever a bundle releases the Coordinator service.

Comment 4 John Ross 2011-11-01 16:21:19 EDT

Note that, upon failing an orphaned coordination, participants will receive the CoordinationImpl and not the CoordinationReferent used to register them in the first place. I don't think this matters much, but it seems that CoordinationImpl.equals(CoordinationReferent) should be true?

Comment 5 BJ Hargrave 2011-11-01 16:50:11 EDT

(In reply to comment #4)
> Note that, upon failing an orphaned coordination, participants will receive the
> CoordinationImpl and not the CoordinationReferent used to register them in the
> first place. I don't think this matters much, but it seems that
> CoordinationImpl.equals(CoordinationReferent) should be true?

Make a new CoordinationReferent to pass to the participants. CoordinationReferent must delegate hashCode and equals[1] to the wrapped CoordinationImpl. Therefore the Coordination objects as seen by the participants (CoordinationReferents) will be equal but not identical.

[1] It is not simple delegation for equals in that the CoordinationReferent will need to access the wrapped CoordinationImpl of the other objects and pass that to the equals method of the wrapped CoordinationImpl. So like:

public boolean equals(Object obj) {
	if (obj == this) {
		return true;
	}
	if (!(obj instanceof CoordinationReferent)) {
		return false;
	}
	CoordinationReferent ref = (CoordinationReferent) obj;
	return getCoordinationImpl().equals(ref.getCoordinationImpl());
}

Comment 6 BJ Hargrave 2011-11-01 16:54:20 EDT

(In reply to comment #5)
> I don't think this matters much, but it seems that
> > CoordinationImpl.equals(CoordinationReferent) should be true?

But this is actually a non-issue. Since, by definition, no one has a reference to the original CoordinationReferent object, they can't possibly do an equals. :-)

But we should still create a new CoordinationReferent to pass to the participants. You have no idea what they will do with that object.

Comment 7 John Ross 2011-11-01 17:16:07 EDT

(In reply to comment #6)
> But this is actually a non-issue. Since, by definition, no one has a reference
> to the original CoordinationReferent object, they can't possibly do an equals.
> :-)

I'm so tired of getting tripped up by that... If it's on the queue, you know for sure noone has a reference to it.

> But we should still create a new CoordinationReferent to pass to the
> participants. You have no idea what they will do with that object.

Creating a new CoordinationReferent would be simple and inexpensive. I'm just not sure why we care about passing the CoordinationImpl at this point since the design of only exposing the referent has served its purpose, and we know the coordination will be ended after the participants have been notified of the failure.

You would, of course, pass the existing referent at any other time. Or, actually, is that your point? You're saying we should never let participants have access to the original referent (i.e. the one given to callers of begin or create)?

Comment 8 BJ Hargrave 2011-11-01 17:50:04 EDT

(In reply to comment #7)
> > But we should still create a new CoordinationReferent to pass to the
> > participants. You have no idea what they will do with that object.
> 
> Creating a new CoordinationReferent would be simple and inexpensive. I'm just
> not sure why we care about passing the CoordinationImpl at this point since the
> design of only exposing the referent has served its purpose, and we know the
> coordination will be ended after the participants have been notified of the
> failure.

I would like to consistently never pass the CoordinationImpl to the clients. Always pass a CoordinationReferent.

> 
> You would, of course, pass the existing referent at any other time. Or,
> actually, is that your point? You're saying we should never let participants
> have access to the original referent (i.e. the one given to callers of begin or
> create)?

No. I am saying we should only ever pass CoordinatorReferents to clients. Never CoordinationImpls. Constructing a CoordinationReferent is almost free and we are in an error path anyway.

Comment 9 John Ross 2011-11-02 09:30:00 EDT

(In reply to comment #8)
> (In reply to comment #7)
> > > But we should still create a new CoordinationReferent to pass to the
> > > participants. You have no idea what they will do with that object.
> > 
> > Creating a new CoordinationReferent would be simple and inexpensive. I'm just
> > not sure why we care about passing the CoordinationImpl at this point since the
> > design of only exposing the referent has served its purpose, and we know the
> > coordination will be ended after the participants have been notified of the
> > failure.
> I would like to consistently never pass the CoordinationImpl to the clients.
> Always pass a CoordinationReferent.

For consistency then, okay.

> > 
> > You would, of course, pass the existing referent at any other time. Or,
> > actually, is that your point? You're saying we should never let participants
> > have access to the original referent (i.e. the one given to callers of begin or
> > create)?
> No. I am saying we should only ever pass CoordinatorReferents to clients. Never
> CoordinationImpls. Constructing a CoordinationReferent is almost free and we
> are in an error path anyway.

Fine. I just thought you were initially suggesting that we always pass a new CoordinationReferent to participants for all paths.

Comment 10 John Ross 2011-11-03 11:58:46 EDT

I found it the easiest to set things up as follows.

All methods on Coordinator that return a Coordination or Coordinations will return the same CoordinationReferent object that was returned to the initiator in either the create or begin methods. This means a management agent (or whatever) could maintain a strong reference to the referent even though the initiator let it go. Would it be better to only pass the tracked referent (i.e. the object passed to the WeakReference constructor) to the initiator?

When calling end or fail on a Coordination, a new CoordinationReferent object is created and that object is passed to each Participant as part of ended or failed. This applies to all flows.

Objections?

Comment 11 BJ Hargrave 2011-11-03 12:35:30 EDT

(In reply to comment #10)
> I found it the easiest to set things up as follows.
> 
> All methods on Coordinator that return a Coordination or Coordinations will
> return the same CoordinationReferent object that was returned to the initiator
> in either the create or begin methods. This means a management agent (or
> whatever) could maintain a strong reference to the referent even though the
> initiator let it go. 
> Would it be better to only pass the tracked referent (i.e.
> the object passed to the WeakReference constructor) to the initiator?

So your point here is to pay specific attention to the originally returned (begin, create methods) CoordinationReferent object rather than CoordinationReferents returned through other means. Since we really want to catch the initiator in a position where it cannot properly end the coordination because it failed to maintain a strong reference to the CoordinationReferent.

> 
> When calling end or fail on a Coordination, a new CoordinationReferent object
> is created and that object is passed to each Participant as part of ended or
> failed. This applies to all flows.

peek, pop, getCoordination* are also where we want to use a different CoordinationReferent object than returned to the initiator. Since we don't want those callers to strongly hold a coordination which has been orphaned by its initiator.

> 
> Objections?

I guess this makes sense.

Comment 12 John Ross 2011-11-03 20:38:42 EDT

Created attachment 206438 [details]
Proposed Patch

New CoordinationReferent. It is tracked by the CoordinationWeakReference in order to know when an initiator has abandoned all strong references to the coordination without properly ending it. It is also used as a simple proxy when making a coordination available to clients other than the initiator.

New CoordinationWeakReference. Maintains a strong reference to the CoordinationImpl. When it is placed on the ReferenceQueue, the CoordinationImpl is retrieved and failed with Coordination.ORPHANED.

A CoordinationReferent is returned to callers of Coordinator.create() and begin(). Only instances returned by these methods are tracked by CoordinationWeakReference. Other API methods that receive coordinations as results (e.g., Coordinator.getCoordinations()) or parameters (e.g., Participant.failed()), receive different instances.

Note that one of the goals was to minimize the impact of this change on existing code. Consequently, all internal data structures and methods continue to use CoordinationImpl. It is only references to the initiator's instance of CoordinationReferent that must not be strongly referenced by the implementation. When providing coordination instances externally to clients other than the initiator, CoordinationImpl is wrapped with a new CoordinationReferent.

Coordinations will remain visible to callers of relevant methods (other than create() or begin()) until the orphaned coordination has been removed by the fail process, even when CoordinationWeakReference.get() would return null. This makes the flow of orphaned coordinations seamless with others. A management agent, for example, might detect that a coordination is obsolete and fail it before the orphan cleanup process executes. CoordinationWeakReference instances are transparently tracked by the CoordinationWeakReference class.

Comment 13 John Ross 2011-11-04 10:24:36 EDT

Committed patch to head.

http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=9c2b8346a5d28d5d727273c5c90873e40201f20f

This is the last in a series of three commits.

Comment 14 John Ross 2011-11-04 10:46:28 EDT

Note that I also bumped the version up from 1.0.0 to 1.1.0.

Comment 15 BJ Hargrave 2011-11-07 12:46:37 EST

Review comments:

+ @Override
+ public boolean equals(Object object) {
+   if (object instanceof CoordinationReferent)
+     return coordination.equals(((CoordinationReferent)object).coordination);
+   return coordination.equals(object);
+ }

The final equals is suspect. This method must always return false if object is not an instanceof CoordinationReferent. A better impl is:

if (object == this) return true;
if (!(object instanceof CoordinationReferenct)) return false;
return coordination.equals(((CoordinationReferent)object).coordination);

+ private static final Set<CoordinationWeakReference> references = Collections.synchronizedSet(new HashSet<CoordinationWeakReference>());

What is the point of the references Set? You add and remove from it but never otherwise use the set. Seems like it can be deleted. If the whole point is just to maintain a strong reference to the CoordinationWeakReference objects, that would be better done with a field in CoordinationImpl, no? Once the references Set is deleted, we no longer need that static newInstance method and can just use the constructor.

+ try {
+   c.fail(Coordination.ORPHANED);
+ }
+ catch (Throwable t) {
+   c.getLogService().log(LogService.LOG_ERROR, NLS.bind(Messages.CoordinatorImpl_5, c.getName(), c.getId()), t);
+ }
+ finally {
+   try {
+     c.end();
+   }
+   catch (CoordinationException e) {
+     // This is expected since we already failed the coordination.
+   }
+ }

In the finally block you also need to catch exceptions than CoordinationException. While CoordinationException is expected, only the one for the ORPHANED exception should be discarded. What if another one is thrown? That should be logged. Also any other exception could be thrown. You need to catch and log that.

+ public static String CoordinatorImpl_5;

Can we please use meaningful variable names for these messages? Fix all of them.

Comment 16 BJ Hargrave 2011-11-07 12:52:11 EST

(In reply to comment #15)
> Review comments:
> 
> + @Override
> + public boolean equals(Object object) {
> +   if (object instanceof CoordinationReferent)
> +     return coordination.equals(((CoordinationReferent)object).coordination);
> +   return coordination.equals(object);
> + }
> 
> The final equals is suspect. This method must always return false if object is
> not an instanceof CoordinationReferent. A better impl is:
> 
> if (object == this) return true;
> if (!(object instanceof CoordinationReferenct)) return false;
> return coordination.equals(((CoordinationReferent)object).coordination);
> 

I suspect you had that last equals for this code:

- while (coordinator.peek() != this) {
+ while (!coordinator.peek().equals(this)) {

Since peek() returns a CoordinationReference and this is a CoordinationImpl. But a correct implementation of equals must return false of the objects being compared are not of the same type. You need to change the while statement to compare two CoordinationImpl objects.

Comment 17 John Ross 2011-11-08 11:47:46 EST

(In reply to comment #15)
> The final equals is suspect. This method must always return false if object is
> not an instanceof CoordinationReferent.

http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=700a442d15ad226b6c762a6adc35507dab45ffcd

> What is the point of the references Set? You add and remove from it but never
> otherwise use the set. Seems like it can be deleted.

http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=b854d31f31eb59741811b41ba9b4b18b47a05128

> In the finally block you also need to catch exceptions than
> CoordinationException. While CoordinationException is expected, only the one
> for the ORPHANED exception should be discarded. What if another one is thrown?
> That should be logged. Also any other exception could be thrown. You need to
> catch and log that.

http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=42a48979538c0fb268cd569ed24d9736db201fc1

> Can we please use meaningful variable names for these messages? Fix all of
> them.

http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=ef3fa6e650dbe7273d2721dd85d92296072a374d(In reply to comment #16)

> (In reply to comment #15)
> But a correct implementation of equals must return false of the objects being
> compared are not of the same type. You need to change the while statement to
> compare two CoordinationImpl objects.

http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=700a442d15ad226b6c762a6adc35507dab45ffcd


Please double-check my logic regarding the relationships between CoordinationImpl, CoordinationReferent, and CoordinationWeakReference. Particularly with regard to how CoordinationImpl and CoordinationReferent are constructed and the CoordinationWeakReference field in CoordinationImpl and Coordinator.begin().

Comment 18 BJ Hargrave 2011-11-10 11:02:52 EST

Looks good now. I also released http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=1bac1c26602238c3a91d9d85bd1482eb63dd749d to ensure we don't leak the CoordinationImpl objects and only provide CoordinationReferent objects.

Comment 19 John Ross 2011-11-15 06:44:28 EST

I used this bug to also remove the unnecessary x-internal package import for org.eclipse.equinox.coordinator from the manifest.

http://git.eclipse.org/c/equinox/rt.equinox.bundles.git/commit/?id=8f4febc65c51d2bd1af852843c107475a88ac0d6

I'll keep this open until I've updated the OSGi repository with this week's i-build.

Comment 20 John Ross 2011-11-15 08:26:32 EST

Fixed in master.