SyncronizingProxyFactory: Pattern or antipattern?

[Daniel Pitts]

I'm creating several interfaces (which actually may be remote). The
implementations need to perform several operations "atomically" on one
target object.
I thought that it might be annoying to write stuff like:
synchronized(target) { target.doOp(); target.doOp2(); }
over and over again. So I thought to borrow a concept from Aspect
Oriented Programming, and wrote myself a proxy factory.

If my interfaces extend java.rmi.Remote, does this preclude me from
using this Proxy class?
<sscce>
import java.lang.reflect.Proxy;
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.Method;
import java.lang.reflect.InvocationTargetException;

/**
*/
public class SynchronizingProxyFactory {
private static class SynchronizedInvocationHandler<E> implements
InvocationHandler {
private final E target;
private final Object sync;

public SynchronizedInvocationHandler(E target, Object sync) {
this.target = target;
this.sync = sync;
}

public Object invoke(Object proxy, Method method, Object[] args)
throws Throwable{
synchronized(sync) {
try {
return method.invoke(target, args);
} catch (InvocationTargetException e) {
throw e.getTargetException();
}
}
}
}
public static <E> E synchronizedProxy(Class<E> iface, E target,
Object sync) {
return (E)Proxy.newProxyInstance(iface.getClassLoader(),
new Class[]{iface}, new
SynchronizedInvocationHandler<E>(target, sync)) ;
}
}
</sscce>

Thanks,
Daniel.

 

[Chris Uppal]

I thought that it might be annoying to write stuff like:
synchronized(target) { target.doOp(); target.doOp2(); }
over and over again. So I thought to borrow a concept from Aspect
Oriented Programming, and wrote myself a proxy factory.

I worry a little when concepts like automatic synchronising wrappers turn up.
The problem is that the appropriate granularity of synchronisation depends on
the meaning of the operations invoked by the API, and by their interrelations,
rather than by the boundaries between the actual methods in the API.

If the API is unusually well-defined, then each semantic operation will
correspond to exactly one method call (which is a valuable property for an API
in itself, and even more so when API calls involve moving data over a
network[*]). But if not -- as is typical -- then automatically adding
"synchronize" to each method call will not provide the necessary safety.

In your example:

synchronized(target) { target.doOp(); target.doOp2(); }

would be implemented by the automatic wrapper as if the code said:

synchronized(target) { target.doOp(); }
synchronized(target) { target.doOp2(); }

which doesn't provide the same semantic guarantees at all.

The converse of the same point is that the locking behaviour of an application
is not something that can be fully encapsulated and hidden away. It is an
issue that can have global ramifications for the design of an application,
unless you want to risk poor performance or even deadlocks. In non threaded
code you can (putting things in OO terms) just ask some object to do something,
and let it worry about the implementation -- so the large-scale application
architecture is not dependent on the details of how the object implements that
operation -- but that is doesn't always apply where locks are concerned, since
any held lock can potentially lock out any other piece of code. That's not
always a problem, but where it is, I'd worry that low-level, automated
facilities would not be "intelligent" enough to do the job properly.


The greatest danger (IMO) is that the code which uses the wrapper /may/ be
correctly designed to use synchronisation properly, or it may not, and there is
no way to tell which just by reading it. I.e. there is no way to tell
easily -- or even make a good guess -- whether the implicit synchronisation is
any of:
+ correct by carefully considered design
+ correct by unconsidered default
+ correct but unnecessarily slow
+ incorrect

That worry is greater still when there may be more than one proxy for the same
object. In normal circumstances, multiple proxies are either a complete no-no
(which may apply in your case, though you don't mention it); or are considered
to be a normal state of affairs. If your semantic safeness depends on the
proxies all agreeing on how they synchronise, then you may have to put more
work into that part of the design than you've shown so far (presumably the
separation of the 'target' and 'sync' objects in the wrapper API is intended to
be a step in that direction).

The only case I can think of (so far) where the automatic synchronised wrappers
might be worthwhile is if the wrapper is /only/ intended to protect the state
of the network proxy object itself, not the state of the object it is a proxy
/for/. (In which case method-level boundaries would be appropriate -- but then
I'd sort of hope the RMI implementation would have that under control anyway).

-- chris

 

[Daniel Pitts]

I thought that it might be annoying to write stuff like:
synchronized(target) { target.doOp(); target.doOp2(); }
over and over again. So I thought to borrow a concept from Aspect
Oriented Programming, and wrote myself a proxy factory.

I worry a little when concepts like automatic synchronising wrappers turn up.
The problem is that the appropriate granularity of synchronisation depends on
the meaning of the operations invoked by the API, and by their interrelations,
rather than by the boundaries between the actual methods in the API.

If the API is unusually well-defined, then each semantic operation will
correspond to exactly one method call (which is a valuable property for an API
in itself, and even more so when API calls involve moving data over a
network[*]). But if not -- as is typical -- then automatically adding
"synchronize" to each method call will not provide the necessary safety.

In your example:

synchronized(target) { target.doOp(); target.doOp2(); }

would be implemented by the automatic wrapper as if the code said:

synchronized(target) { target.doOp(); }
synchronized(target) { target.doOp2(); }

which doesn't provide the same semantic guarantees at all.

The converse of the same point is that the locking behaviour of an application
is not something that can be fully encapsulated and hidden away. It is an
issue that can have global ramifications for the design of an application,
unless you want to risk poor performance or even deadlocks. In non threaded
code you can (putting things in OO terms) just ask some object to do something,
and let it worry about the implementation -- so the large-scale application
architecture is not dependent on the details of how the object implements that
operation -- but that is doesn't always apply where locks are concerned, since
any held lock can potentially lock out any other piece of code. That's not
always a problem, but where it is, I'd worry that low-level, automated
facilities would not be "intelligent" enough to do the job properly.

The greatest danger (IMO) is that the code which uses the wrapper /may/ be
correctly designed to use synchronisation properly, or it may not, and there is
no way to tell which just by reading it. I.e. there is no way to tell
easily -- or even make a good guess -- whether the implicit synchronisation is
any of:
+ correct by carefully considered design
+ correct by unconsidered default
+ correct but unnecessarily slow
+ incorrect

That worry is greater still when there may be more than one proxy for the same
object. In normal circumstances, multiple proxies are either a complete no-no
(which may apply in your case, though you don't mention it); or are considered
to be a normal state of affairs. If your semantic safeness depends on the
proxies all agreeing on how they synchronise, then you may have to put more
work into that part of the design than you've shown so far (presumably the
separation of the 'target' and 'sync' objects in the wrapper API is intended to
be a step in that direction).

All of those are good points.

A little more background for my particular project might help.

I have a class "Game", which contains at least one member of type
"GameState". GameState is an abstract class, implementing the "State/
Strategy" pattern. Certain method calls are only valid with certain
states. The actual value of the GameState object depends on the valid
operations on Game.

So, here comes the synchronizing design...

Operations on Game need to be atomic, and Game is already large enough
that I'd prefer not to delegate to GameState within game. I created a
facade class "GameController" which references a "Game" object. There
can be several GameController objects per Game object. GameController
delegates to a few methods in Game, and then all methods in
game.getGameState(). I am asserting that each method on
GameController needs to be atomic to the given Game object. Hence the
target/sync in my proxy.

My target would the GameController, and my sync would be the Game.
This allows the GameController to behave correctly even if there are
more than one acting on the same Game.

I realized that my code would have to be constructed with the concept
that the method call itself was the atomic operation, not a group of
them. This is knowledge that is important anyway, so if someone
doesn't understand this, then they are likely to write incorrect code
with and without my proxy, I think.

To put it another way. my GameController only exposes methods which
are transactional. It is also only invoked by a user performing an
action in a GUI. The operations themselves are fast enough, and
spread far enough apart, that the synchronization isn't going to harm
performance, and is actually required for proper operation.

The only case I can think of (so far) where the automatic synchronised wrappers
might be worthwhile is if the wrapper is /only/ intended to protect the state
of the network proxy object itself, not the state of the object it is a proxy
/for/. (In which case method-level boundaries would be appropriate -- but then
I'd sort of hope the RMI implementation would have that under control anyway).

Actually, I think you've got this backwards...
The GameController would be proxied to a threadSafeGame. The
threadSafeGame object stays on the server side, and then a
gameControllerRemoteStub is sent to the Client side... The Stub is
actually a proxy for the threadSafeGame, and the threadSafeGame a
proxy for the gameController. Calls on the client side may be serial,
but they might be parallel as well. the threadSafeGame makes them all
serial with regards to the game object.

Does this design make more sense? Or do you still think its an
antipattern?

 

[Chris Uppal]

I have a class "Game", which contains at least one member of type
"GameState". GameState is an abstract class, implementing the "State/
Strategy" pattern. Certain method calls are only valid with certain
states. The actual value of the GameState object depends on the valid
operations on Game.

OK. That makes sense.

I created a
facade class "GameController" which references a "Game" object. There
can be several GameController objects per Game object. GameController
delegates to a few methods in Game, and then all methods in
game.getGameState().

If GameController is talking directly to the GameState then your design is not,
after all, a variant on the State pattern (or Strategy, come to that), and I no
longer feel I understand what the design is. That's not to suggest that
there's anything wrong with the design, but take my remaining comments with
corresponding caution.

To put it another way. my GameController only exposes methods which
are transactional. It is also only invoked by a user performing an
action in a GUI. The operations themselves are fast enough, and
spread far enough apart, that the synchronization isn't going to harm
performance, and is actually required for proper operation.

It sounds as if you GameController has responsibility for grouping primitive
operations on Game into semantically/transactionally atomic units. If so, then
I question whether it's appropriate to hide that -- critical -- aspect of its
function in the implicit synchronisation provided by a SynchronisingWrapper
/around/ a GameController.

Does this design make more sense? Or do you still think its an
antipattern?

Let me put it this way: I'm /less/ skeptical now ;-)

-- chris

 

[Esmond Pitt]

Is there a reason why you don't just make all the GameController methods
synchronized? as they have to be atomic?

 

[Chris Uppal]

Is there a reason why you don't just make all the GameController methods
synchronized? as they have to be atomic?

AIUI, they have to be atomic with respect to their common Game object, so just
tagging their methods' declarations with 'synchronized' wouldn't do the trick.

-- chris

 

[Daniel Pitts]

OK. That makes sense.


If GameController is talking directly to the GameState then your design is not,
after all, a variant on the State pattern (or Strategy, come to that), and I no
longer feel I understand what the design is. That's not to suggest that
there's anything wrong with the design, but take my remaining comments with
corresponding caution.


It sounds as if you GameController has responsibility for grouping primitive
operations on Game into semantically/transactionally atomic units. If so, then
I question whether it's appropriate to hide that -- critical -- aspect of its
function in the implicit synchronisation provided by a SynchronisingWrapper
/around/ a GameController.


Let me put it this way: I'm /less/ skeptical now ;-)

-- chris

Okay, let me try to clarify the design further...

The Game object contains a method GameState getState().

GameControllerImpl has a reference to a Game object. each method on
GameController either calls a method on Game, or a method on
game.getState(). Calling, say, game.getState().deal() CAN change the
current GameState object referenced in Game. So, by synchronizing
against the Game object, I protect the game's state, as well as the
game's gameState reference.

The GameController interface is ALSO used as an RMI interface/GUI
facade.

Thats why I say I use the State pattern. The behaviour of the object
returned by game.getState() is likely different every time.