4

I have a (Java) class, WindowItem, that has a problem: One of the methods is not thread-safe. I can't fix WindowItem, because it's part of an external framework. So I figured I implement a Decorator for it, that has a "synchronized" keyword on the method in question.

The Decorator extends WindowItem and will also contain WindowItem. Following the Decorator pattern, I create methods in the Decorator that call the WindowItem it contains.

However, WindowItem has a few final methods, that I cannot override in the Decorator. That breaks the transparency of the Decorator. Let's make this explicit:

public class WindowItem {
   private List<WindowItem> windows;

   public Properties getMethodWithProblem() {
      ...
   }

   public final int getwindowCount() {
      return windows.size();
  }
}

public class WindowItemDecorator extends WindowItem {
   private WindowItem item;

   public WindowItemDecorator(WindowItem item) {
      this.item = item;
   }

   # Here I solve the problem by adding the synchronized keyword:
   public synchronized Properties getgetMethodWithProblem() {
      return super.getMethodWithProblem();
   }

   # Here I should override getWindowCount() but I can't because it's final
}

In my own code, whenever I have to pass a WindowItem somewhere, I wrapped it in a decorator first: new WindowItemDecorator(item) -- and the thread-safety problem disappears. However, if my code calls getwindowCount() on a WindowItemDecorator, it will always be zero: It executes getWindowCount() on the superclass instead of the "item" member.

So I would say the design of WindowItem (the fact that it has public final methods) makes it impossible to create a Decorator for this class.

Is that correct, or am I missing something?

In this case I might be able to keep a copy of the list of windows in the decorator, and keep it in sync, and then the result of getWindowCount() would be correct. But in that case, I prefer to fork and patch the framework...

Paul
  • 1,939
  • 1
  • 18
  • 27
  • 1
    Your first reflex is the right one but having the final methods makes it impossible to override them. Do you have to pass around/manipulate objects of type "WindowItem"? If not, you don't have to use the Decorator pattern. You can use composition and control the calls to the final methods of WindowItem by wrapping them in your new class that doesn't extend WindowItem but uses an instance via composition. – Ushox Aug 02 '12 at 10:08

4 Answers4

2

How about not thinking of the problem this way? Why not just handle the threading issues in your code, without assuming thread-safety of WindowItem.

// I personally prefer ReadWriteLocks, but this sounds like it will do...
synchronized (windowItem) {
    windowItem.getMethodWithProblem();
}

And then submit an RFE with the package maintainer to better support thread safety.

Indeed, if the class isn't designed to be thread safe, it is unlikely that a few synchronized keywords are going to truly fix things. What somebody means by "thread safe" is always relative ;-)

(Incidentally, WindowItem is definitely NOT thread safe as it is using List instead of explicitly using a "thread ready" variant Correct way to synchronize ArrayList in java - there are also no guarantees that the List is being accessed in a thread safe manner).

Community
  • 1
  • 1
fommil
  • 5,757
  • 8
  • 41
  • 81
  • 1
    ... and ask them to implement an interface too, while you're at it. It never hurts (especially for mocking in unit tests). – Duncan Jones Aug 02 '12 at 10:08
  • If `interface`s are too heavyweight then at least remove the `final` modifier from such a trivial method. Or, at least be consistent http://stackoverflow.com/questions/218744 – fommil Aug 02 '12 at 10:14
  • The point of solving this problem using a decorator though, is that I solve the problem in one place, rather than having to fix all the uses of WindowItem in my code. – Paul Aug 02 '12 at 10:18
  • 1
    yeah, but sometimes we just got to take our medicine and admit that the underlying library is not thread safe. I'm not entirely convinced that synchronizing this one method would even fix that for you. For a start, `windows` isn't even `volatile`, never mind `final`, so `getwindowCount` can't **ever** be threadsafe unless the maintainer changes their code. – fommil Aug 02 '12 at 10:24
1

Perhaps you could employ the Delegation Pattern, which would work nicely if the WindowItem class implements an interface defining all the methods you care about. Or if it doesn't break too much of your existing code to refer to this delegated class rather than WindowItem.

Duncan Jones
  • 67,400
  • 29
  • 193
  • 254
  • the problem is more subtle than that - there is no `interface` for `WindowItem` and he is handed the `WindowItem`s. – fommil Aug 02 '12 at 09:55
  • I believe what you're saying is to create a Decorator for an interface of WindowItem instead of for the class itself. I would agree to that, but in this case, there is no such interface. – Paul Aug 02 '12 at 09:56
  • @Paul Swing and a miss. There's just not enough interfaces in the world is there?! – Duncan Jones Aug 02 '12 at 10:04
0

The answer to your question is yes, you can't override final methods, meaning that it's not possible to create a decorator for this class.

If you can override the method that has the problem, and solve the problem by synchronizing the method, you could just leave it at that. That is, just use your subclass, and not use the decorator pattern.

Daniel
  • 10,115
  • 3
  • 44
  • 62
  • Thanks. This is a good solution if I were creating the WindowItem objects. Then, instead of creating a WindowItem object, I create a SyncedWindowItem, problem solved. Unfortunately, the WindowItem objects are created for me (it's not even a concrete class, it's abstract), so I can't do that on already existing objects. – Paul Aug 02 '12 at 09:47
-1

A coworker had an idea that I think can solve the problem. I can keep the state of the superclass and the state of the "item" member in sync by looking at all methods that modify the List windows. There are a few: addWindow, removeWindow. Instead of calling just "item.addWindow(...)" in the decorator, I call addWindow on the superclass as well:

Normal decorator:

public void addWindow(WindowItem newItem) {
   item.addWindow(newItem);
}

In this case I do:

public void addWindow(WindowItem newItem) {
   super.addWindow(newItem);
   item.addWindow(newItem);
}

That keeps the state in sync and the return values of the final methods correct.

This is a solution that can work or not work depending on internals of the class being decorated.

Paul
  • 1,939
  • 1
  • 18
  • 27
  • It's not a general solution, but in this case I believe it works, because I don't need to override any final methods if the internal state is in sync (in this case!). Note that the final method is getWindowCount(). If I make sure that the superclass has the same number of windows at the "item", then there is no need to override getWindowCount(). – Paul Aug 02 '12 at 10:04
  • so basically, you're keeping two copies of the instance - one as delegate and the other as super - so that one returns the right answer for one method, and the other the right answer for all other methods plus the added thread safety logic? Enjoy maintaining that! :-) – fommil Aug 02 '12 at 10:10
  • Incidentally, you'll potentially have broken the thread safety of the class (assuming it had any) by having two versions which are prone to [race conditions](http://en.wikipedia.org/wiki/Race_condition). And I trust nothing outside of your control ever has a reference to the original `WindowItem`? – fommil Aug 02 '12 at 10:19
  • No, I'm not going to enjoy maintaining that. But then again, even a plain old regular decorator is terrible for maintenance, because if a public method is added to the WindowItem class, one needs to remember to add it to the decorator. Which you won't. The reason, I think, you would still use a decorator is that the alternative is worse, which is to fix the bug in the framework, and maintain _that_. – Paul Aug 02 '12 at 12:42
  • *au contraire*! [`@Delegate`](http://projectlombok.org/features/Delegate.html). And don't forget the *other* alternative, which is to maintain your own code without the assumption that the class is thread safe. – fommil Aug 02 '12 at 12:51