Is a Download class a bad candidate for immutability?

Question

I have a class that i use to do downloads. The class is observable and i use it as a swing UI model object too. However it is too slow, both to start (from the http handshaking i presume so not much i can do about it probably) and from synchronized access. The interface of the class is:

public File getDownloadedFile() 
public String getMimeType() 
public synchronized BigInteger getExpectedSize() 
public URL getURL()
public synchronized boolean cancel()
public synchronized boolean retry()
public synchronized boolean isDone()
public synchronized boolean isCancelled() 
public synchronized BigInteger getProgress() 
public String getName() 
public synchronized BigInteger getBytesRead() 
public synchronized BigDecimal getBytesPerSecond() 
public synchronized BigDecimal getTimeRemaining() 
public synchronized void start()

I want to eliminate the synchronized(s) there, but i know that to do so, i have to make a way to turn this class immutable. I tried to model this as a state machine (hidden internally), however the middle (running) state has a InputStream + a thread. What is the correct way to turn this into a immutable wrapper? 1) Make class into a wrapper for a state machine + the immutable state. Make start start the first machine state, then notify observers on each state change with a immutable copy. (however the client expectation of a monolitic class - for the model of the swing ui for ex, is problematic.

2) Give up on using the wrapper as swing model. just register my controller as a observer, start and add the first (immutable) state as a model and use a immutable state identifier (for example the download url) to recognize the download state machine state emitted by notifyObservers() as belonging to some download, to replace each successive state + update the ui. However i don't think this is very usable when this is used without interest in notification. At least a way to wait for the download to be complete would be nice no?

Answer 1

From my perspective if you were to make it so that this class can only download one file it is basically immutable. Yes some of the state may change as a by product of doing its work, but all of the changes are internal. Strictly speaking it is not immutable since it has state that changes, but from a practical point of view it is immutable since it has no methods that allow a programmer to alter those states.

I think I do not quite understand your overall goal. Is it so that the code that is reporting the remaining download time is getting a fixed snapshot of the time remaining (for example) or is it so that the method displaying the file name cannot also cancel the download (for example)?

Answer 2

The way that API is structured means that this class has to encapsulate mutable state somehow.

However, just because the class is mutable, that doesn't mean you need to slap a synchronized keyword on every method.

You should be able to limit your need to synchronize to much smaller blocks inside the code - or even eliminate it altogether by using some of the Atomic classes.

I'd strongly recommend reading Java Concurrency in Practice, which I consider the definitive work on implementing concurrency in java programs. Sure, it doesn't shine a light into the darkest corners, but it will certainly help you understand what is safe, what is unsafe, and what you should avoid without significant preparation...

Answer 3

I think your first mistake is trying to turn this class into a Swing model. Looking at the method list, it appears that the only reason you've done this is to let some Swing component interrogate the object to report progress. That violates the "tell, don't ask" principle of OO design.

So create a separate DownloadProgress object, and have your Download object update it on a regular basis. Since this object just contains data fields, its accessors can be synchronized. If you want it to push out notifications to the Swing component (again, tell don't ask), then your accessors queue up a callback using SwingUtilities.invokeLater().

That leaves controlling the actual download. If you use a Semaphore, then you can set it from one thread (I'll assume the Swing event dispatch thread) and read it from another (the thread that's actually doing the reading). You may have to take more drastic action, such as closing the socket underneath the reader. However, that's a completely different topic from synchronization.

Oh yeah, you are performing the download in a background thread, right? Not on the Swing event dispatch thread?

Answer 4

A good answer. And you're right on the money that this class is being used as a query for the renderer. Right now i "solved" making it immutable by holding each mutable property on a immutable (state-machine) volatile object (that gets updated on each update from the (volatile) stream on another thread). So getting this object from the stream (once) and the (ServiceLoader instantiated - so a form of DI) renderer is querying it for the appropriate fields - information - to choose what to render on my JDownloadList component. Even though your other advice is sound "tell don't ask" i'm not going to follow through on it, just to keep the flexibility of different renderers to use the download information differently without a lot of additional setter on the renderer (that has the download state anyway). I just have to be careful to only ask for the object once at each render and i should be able to remove the synchronized.

Right? Right?