34

In a library in Java 7, I have a class which provides services to other classes. After creating an instance of this service class, one method of it may be called several times (let’s call it the doWork() method). So I do not know when the work of the service class is completed.

The problem is the service class uses heavy objects and it should release them. I set this part in a method (let’s call it release()), but it is not guaranteed that other developers will use this method.

Is there a way to force other developers to call this method after completing the task of service class? Of course I can document that, but I want to force them.

Note: I cannot call the release() method in the doWork() method, because doWork() needs those objects when it is called in next.

  • 46
    What you are doing there is a form of temporal coupling and typically considered a code smell. You may want to force yourself to come up with a better design instead. – Frank Mar 28 '17 at 5:22
  • 1
    @Frank I'd agree, if changing the design of the underlying layer is an option then that would be the best bet. But I suspect this is a wrapper around someone else's service. – Dar Brett Mar 28 '17 at 5:29
  • Which kind of heavy objects your Service needs? If the Service class is not able to manage those resources automatically by itself (without requiring temporal coupling) then perhaps those resources should be managed elsewhere and injected to the Service. Have you written unit tests for the Service class? – COME FROM Mar 28 '17 at 8:01
  • 18
    It is very "un-Java" to require that. Java is a language with garbage collection and now you come up with some kind of contraption in which you require developers to "clean up". – Pieter B Mar 28 '17 at 10:58
  • 22
    @PieterB why? AutoCloseable was made for this exact purpose. – BgrWorker Mar 28 '17 at 14:32
48

The pragmatic solution is to make the class AutoCloseable, and provide a finalize() method as a backstop (if appropriate ... see below!). Then you rely on users of your class to use try-with-resource or call close() explicitly.

Of course I can document that, but I want to force them.

Unfortunately, there is no way1 in Java to force the programmer to do the right thing. The best you could hope to do is to pick up incorrect usage in a static code analyser.


On the topic of finalizers. This Java feature has very few good use cases. If you rely on finalizers to tidy up, you run into the problem that it can take a very long time for the tidy-up to happen. The finalizer will only be run after the GC decides that the object is no longer reachable. That may not happen until the JVM does a full collection.

Thus, if the problem you are tying to solve is to reclaim resources that need to be released early, then you have missed the boat by using finalization.

And just in case you haven't got what I am saying above ... it is almost never appropriate to use finalizers in production code, and you should never rely on them!


1 - There are ways. If you are prepared to "hide" the service objects from user code or tightly control their lifecycle (e.g. https://softwareengineering.stackexchange.com/a/345100/172) then the users code doesn't need to call release(). However, the API becomes more complicated, restricted ... and "ugly" IMO. Also, this is not forcing the programmer to do the right thing. It is removing the programmer's ability to do the wrong thing!

  • 1
    Finalizers teach a very bad lesson - if you screw it, I'll fix it for you. I prefer netty's approach -> a configuration parameter that instructs the (ByteBuffer) factory to randomly create with probability of X% bytebuffers with a finalizer that prints the location where this buffer was acquired (if not already released). So in production this value can be set to 0% - i.e no overhead, and during testing & dev to a higher value like 50% or even 100% in order to detect the leaks before releasing the product – Svetlin Zarev Mar 28 '17 at 12:56
  • 11
    and remember that finalizers are not guaranteed to ever run, even if the object instance is being garbage collected! – jwenting Mar 28 '17 at 13:03
  • 3
    It's worth noting that Eclipse emits a compiler warning if an AutoCloseable is not closed. I'd expect other Java IDE to do so, as well. – meriton - on strike Mar 28 '17 at 22:15
  • 1
    @jwenting In what case don't they run when the object is GC'd? I couldn't find any resource stating or explaining that. – Cedric Reichenbach Mar 29 '17 at 6:28
  • 3
    @Voo You misunderstood my comment. You have two implementations -> DefaultResource and FinalizableResource which extends the first one and adds a finalizer. In production you use DefaultResource which does not have finalizer->no overhead. During development and testing you configure the app to use FinalizableResource which has a finalizer and hence adds overhead. During dev and testing that's not an issue, but it can help you identify leaks and fix them before reaching production. Yet if a leaks reaches PRD you can configure the factory to create X% FinalizableResource to id the leak – Svetlin Zarev Mar 29 '17 at 7:56
55

This seems like the use-case for AutoCloseable interface and the try-with-resources statement introduced in Java 7. If you have something like

public class MyService implements AutoCloseable {
    public void doWork() {
        // ...
    }

    @Override
    public void close() {
        // release resources
    }
}

then your consumers can use it as

public class MyConsumer {
    public void foo() {
        try (MyService myService = new MyService()) {
            //...
            myService.doWork()
            //...
            myService.doWork()
            //...
        }
    }
}

and not have to worry about calling an explicit release regardless of whether their code throws an exception.


Additional answer

If what you are really looking for is to ensure that no one can forget to use your function, you have to do a couple of things

  1. Ensure all constructors of MyService are private.
  2. Define a single point of entry to use MyService that ensures it is cleaned up after.

You would do something like

public interface MyServiceConsumer {
    void accept(MyService value);
}

public class MyService implements AutoCloseable {
    private MyService() {
    }


    public static void useMyService(MyServiceConsumer consumer) {
        try (MyService myService = new MyService()) {
            consumer.accept(myService)
        }
    }
}

You would then use it as

public class MyConsumer {
    public void foo() {
        MyService.useMyService(new MyServiceConsumer() {
            public void accept(MyService myService) {
                myService.doWork()
            }
        });
    }
}

I did not recommend this path originally because it is hideous without Java-8 lambdas and functional interfaces (where MyServiceConsumer becomes Consumer<MyService>) and it is fairly imposing on your consumers. But if what you only want is that release() must get called, it will work.

  • 1
    This answer is probably better than mine. My way has a delay before the Garbage Collector kicks in. – Dar Brett Mar 28 '17 at 5:35
  • 1
    How do you ensure that clients will use try-with-resources and not just omit the try, thus missing the close call? – Frank Mar 28 '17 at 6:11
  • @walpen This way has the same problem. How I can force the user to use try-with-resources? – hasanghaforian Mar 28 '17 at 6:29
  • 1
    @Frank / @hasanghaforian, Yes, this way does not force close to be called. It does have the benefit of familiarity: Java developers know you really should make sure .close() is called when the class implements AutoCloseable. – walpen Mar 28 '17 at 6:55
  • 1
    Couldn't you pair a finalizer with a using block for deterministic finalization? At least in C#, this becomes a pretty clear pattern for developers to get in the habit of using when leveraging resources that need deterministic finalization. Something easy and habit forming is the next best thing when you can't force someone to do something. stackoverflow.com/a/2016311/84206 – AaronLS Mar 28 '17 at 11:58
52

Yes you can

You can absolutely force them to release, and make it so it is 100% impossible to forget, by making it impossible for them to create new Service instances.

If they did something like this before:

Service s = s.new();
try {
  s.doWork("something");
  if (...) s.doWork("something else");
  ...
}
finally {
  s.release(); // oops: easy to forget
}

Then change it so they must access it like this.

// Their code

Service.runWithRelease(new ServiceConsumer() {
  void run(Service s) {
    s.doWork("something");
    if (...) s.doWork("something else");
    ...
  }  // yay! no s.release() required.
}

// Your code

interface ServiceConsumer {
  void run(Service s);
}

class Service {

   private Service() { ... }      // now: private
   private void release() { ... } // now: private
   public void doWork() { ... }   // unchanged

   public static void runWithRelease(ServiceConsumer consumer) {
      Service s = new Service();
      try {
        consumer.run(s);
      }
      finally {
        s.release();
      } 
    } 
  }

Caveats:

  • Consider this pseudocode, it has been ages since I've written Java.
  • There might be more elegant variants around these days, maybe including that AutoCloseable interface someone mentioned; but the given example should work out of the box, and except for elegantness (which is a nice goal in itself) there should be no major reason to change it. Note that this intends to say that you could use AutoCloseable inside your private implementation; the benefit of my solution over having your users use AutoCloseable is that it can, again, not be forgotten.
  • You'll have to flesh it out as needed, for example to inject arguments into the new Service call.
  • As mentioned in the comments, the question of whether a construct like this (i.e., taking the process of creating and destroying a Service) belongs in the hand of the caller or not is outside of the scope of this answer. But if you decide that you absolutely need this, this is how you can do it.
  • One commenter provided this information regarding exception handling: Google Books
  • 2
    I am happy about comments for the downvote, so I can improve the answer. – AnoE Mar 28 '17 at 15:30
  • 2
    I didn't downvote, but this design is likely to be more trouble than it's worth. It adds a great deal of complexity and imposes a large burden on callers. Developers should be familiar enough with try-with-resources style classes that using them is second nature whenever a class implements the appropriate interface, so that is typically good enough. – jpmc26 Mar 28 '17 at 16:08
  • 30
    @jpmc26, funnily I feel that that approach reduces complexity and burden on the callers by saving them from thinking about the resource lifecycle at all. Aside from that; the OP did not ask "should I force the users..." but "how do I force the users". And if it is important enough, this is one solution that works very well, is structurally simple (just wrapping it in a closure/runnable), even transferable to other languages which have lambdas/procs/closures as well. Well, I'll leave it to the SE democracy to judge; the OP has already decided anyways. ;) – AnoE Mar 28 '17 at 16:13
  • 14
    Again, @jpmc26, I'm not that much interested in discussing whether this approach is correct for any single use case out there. The OP asks how to enforce such a thing and this answer tells how to enforce such a thing. It is not for us to decide whether it is appropriate to do so in the first place. The technique shown is a tool, nothing more, nothing less, and useful to have in ones bag-of-tools, I believe. – AnoE Mar 28 '17 at 19:46
  • 11
    This is the correct answer imho, it is essentially the hole-in-the-middle pattern – jk. Mar 29 '17 at 7:41
11

You could try to make use of the Command pattern.

class MyServiceManager {

    public void execute(MyServiceTask tasks...) {
        // set up everyting
        MyService service = this.acquireService();
        // execute the submitted tasks
        foreach (Task task : tasks)
            task.executeWith(service);
        // cleanup yourself
        service.releaseResources();
    }

}

This gives you full control over resource acquisition and release. The caller only submits tasks to your Service, and you yourself are responsible for acquiring and cleaning up resources.

There is a catch, however. The caller can still do this:

MyServiceTask t1 = // some task
manager.execute(t1);
MyServiceTask t2 = // some task
manager.execute(t2);

But you can adress this problem when it arisis. When there are performance problems and you find out that some caller do this, simply show them the proper way and resolve the issue:

MyServiceTask t1 = // some task
MyServiceTask t2 = // some task
manager.execute(t1, t2);

You can make this arbitrarly complex by implementing promises for tasks that are dependent on other tasks, but then releasing stuff also gets more complicated. This is only a starting point.

Async

As has been pointed out in the comments, the above doesn't really work with asynchronous requests. Thats correct. But this can easily be solved in Java 8 with the use of CompleteableFuture, especially CompleteableFuture#supplyAsync to create the individual futures and CompleteableFuture#allOf to perfom the release of the resources once all tasks have finished. Alternatively, one can always use Threads or ExecutorServices to roll their own implementation of Futures/Promises.

  • 1
    I would appreciate it if downvoters leave a comment why they think this is bad, so that I can either adress those concerns directly or at least get another point of view for the future. Thanks! – Polygnome Mar 28 '17 at 15:56
  • +1 upvote. This may be an approach, but service is async and the consumer needs to get the first result before asking for the next service. – hasanghaforian Mar 28 '17 at 18:43
  • 1
    This is just a barebones template. JS solves this with promises, you could do similar things in Java with futures and a collector that is called when all tasks are finished and then cleans up. Its definitely possible to get async and even dependencies in there, but it also complicates things a lot. – Polygnome Mar 28 '17 at 22:00
3

If you can, do not allow the user to create the resource. Let the user pass a visitor object to your method, which will create the resource, pass it to the visitor object's method, and release afterwards.

  • Thank you for your reply, but excuse me, do you mean that it is better to pass resources to the consumer and then get it again when he ask for service? Then the problem will remain: Consumer may forget to release those resources. – hasanghaforian Mar 28 '17 at 18:53
  • If you want to control a resource, don't let go of it, don't pass it to someone else's flow of execution completely. One way to do it is to run a predefined method of a user's visitor object. AutoCloseable does a very similar thing behind the scenes. Under another angle, it's similar to dependency injection. – 9000 Mar 28 '17 at 19:28
1

My idea was similar to Polygnome's.

You can have the "do_something()" methods in your class just add commands to a private command list. Then, have a "commit()" method that actually does the work, and calls "release()".

So, if the user never calls commit(), the work is never done. If they do, the resources are freed.

public class Foo
{
  private ArrayList<ICommand> _commands;

  public Foo doSomething(int arg) { _commands.Add(new DoSomethingCommand(arg)); return this; }
  public Foo doSomethingElse() { _commands.Add(new DoSomethingElseCommand()); return this; }

  public void commit() { 
     for(ICommand c : _commands) c.doWork();
     release();
     _commands.clear();
  }

}

You can then use it like foo.doSomething.doSomethineElse().commit();

  • This is the same solution, but instead of asking the caller to maintain the task list you maintain it internally. The core concept is literally the same. But this does not follow any coding conventions for Java I have ever seen and is actually quite hard to read (loop body on the same line as loop header, _ in the beginning). – Polygnome Mar 29 '17 at 8:21
  • Yes, it is the command pattern. I just put down some code to give a gist of what I was thinking in as concise a way as possible. I mostly write C# these days, where private variables are often prefixed with _. – Sava B. Mar 29 '17 at 20:33
-1

Another alternative to the ones mentioned would be the usage of "smart references" to manage your encapsulated resources in a way which enables the garbage collector to automatically clean up without manually freeing resources. Their usefulness depends of course on your implementation. Read more on this topic in this wonderful blog post: https://community.oracle.com/blogs/enicholas/2006/05/04/understanding-weak-references

  • This is an interesting idea, but I cannot see how using weak references solves the OP's problem. The service class does not know whether it is going to get another doWork() request. If it releases its reference to its heavy objects, and then it gets another doWork() call, it will fail. – Jay Elston Apr 13 '17 at 1:49
-4

For any other Class Oriented language I'd say put it in the destructor, but since that's not an option I think you can override the finalize method. That way when the instance goes out of scope and is garbage collected it will be released.

@Override
public void finalize() {
    this.release();
}

I'm not too familiar with Java, but I think that this is the purpose for which finalize() is intended.

http://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#finalize()

  • 7
    This is a bad solution to the problem for the reason Stephen says in his answer - If you rely on finalizers to tidy up, you run into the problem that it can take a very long time for the tidy-up to happen. The finalizer will only be run after the GC decides that the object is no longer reachable. That may not happen until the JVM does a full collection. – Daenyth Mar 28 '17 at 12:26
  • 4
    And even then the finalizer may not in fact run... – jwenting Mar 28 '17 at 13:04
  • 3
    Finalizers are notoriously unreliable: they may run, they may never run, if they run there is no guarantee when they will run. Even the Java architects such as Josh Bloch say finalizers are a terrible idea (reference: Effective Java (2nd Edition)). – user22815 Mar 28 '17 at 16:43
  • These kind of issues make me miss C++ with its deterministic destruction and RAII... – Mark K Cowan Mar 29 '17 at 16:31

Not the answer you're looking for? Browse other questions tagged or ask your own question.