Can an optimistic try-with-resources cause a memory/resource leak over time?

570 Views Asked by At

Consider the following code:

try (Transport transport = session.getTransport("smtp")) {
    transport.connect();
} catch (MessagingException ignored) {}

What if, for some reason the auto-close fails? That might leave internally some file handler open (maybe not with SMTP but in general)

So is it correct to say that this code is prone to a resource/memory leak?

If the answer's yes, then what should a program do in such case?

2

There are 2 best solutions below

0
rzwitserloot On BEST ANSWER

The answer is 'theoretically, yes, pragmatically, no'. In the sense that:

So is it correct to say that this code is prone to a resource/memory leak?

If this is 'yes', then

If the answer's yes, then what should a program do in such case?

There is absolutely nothing you can do in this hypothetical, silly case.

Hence why it's pragmatically speaking no.

So how can it fail?

The code you wrote is syntax sugar. It's the same as this:

try {
  Transport transport = session.getTransport("smtp");
  try {
    transport.connect();
  } finally {
    try {
     transport.close();
    } catch (Throwable t) {
      if (we got here because the try block threw) {
        theExceptionThatGotUsHere.addSuppressed(t);
        throw theExceptionThatGotUsHere;
      } else {
        throw t;
      }
    }
  }
} catch (MessagingException ignored) {}

You see why java has syntax sugar for that mess. In essence, it just does the same thing we used to do, with the one upside that if close() throws an exception, java takes some pains to ensure this does not end up confuscating the exception thrown by the main body. Generally if the flow goes:

  • Write to some resource a bunch
  • Eventually due to, say, someone yanking a disk right out, an IOException occurs explaining that someone yanked a disk out.
  • We get to the 'resource safety' part of the code which calls close() on an OutputStream that is already tainted and that has already thrown. This, itself, also throws.
  • The first exception is more useful than the second; they are both IOExceptions and will cause the same code flow, just, the first one has a 'better' stack trace and a 'better' message ('better' in the sense that they will more easily lead to fixing the bug).

Hence, perhaps, it's essentially as simple as:

try {
  Transport transport = session.getTransport("smtp");
  try {
    transport.connect();
  } finally {
    transport.close();
  }
} catch (MessagingException ignored) {}

So, how can this fail? Simple: If close() itself can no longer close the resource.

However:

  • That means there is nothing you can do from within java. The 'go to' answer is the very last snippet: Really really try, in a finally block, but that's with try-with-resources already does.
  • This just is not relevant. All java APIs with a close method will close the resource. They just do. "Yeah but what if they don't". Okay. What if the moon is made of cheese?

Specifically, the close() may fail, but what they won't do, is cause a resource/memory leak even if they fail. What DOES happen is the following:

  • Due to circumstances (Such as yanking a disk out, a network router going belly up, etc), a resource becomes 'invalid' and any attempt to interact with it throws exceptions.
  • Notably, so does close(), this is important in particular for output streams, where close() throwing cannot safely be ignored, it means not all data is guaranteed to have arrived where you sent it, you should handle an exception in an output stream's close() call no different than in a write call().
  • The underlying resources that the stream is taking up were already released when it became invalid. For memory leak/resources purposes, you already no longer had to invoke close() on this.
  • Alternatively, the close() call cleans up resources (releases OS-level file handles, that sort of thing), and then throws the exception, to ensure you know that the write did not necessarily succeed.
3
TopchetoEU On

You are correct in your assumption that if an exception is thrown upon the freeing of transport, some memory leak will occur, if the library you're using is bad. In normal circumstances, the AutoCloseable guarantees that all unmanaged resources will be freed, as the Java documentation states:

While this interface method is declared to throw Exception, implementers are strongly encouraged to declare concrete implementations of the close method to throw more specific exceptions, or to throw no exception at all if the close operation cannot fail.

Cases where the close operation may fail require careful attention by implementers. It is strongly advised to relinquish the underlying resources and to internally mark the resource as closed, prior to throwing the exception. The close method is unlikely to be invoked more than once and so this ensures that the resources are released in a timely manner. Furthermore it reduces problems that could arise when the resource wraps, or is wrapped, by another resource.

Implementers of this interface are also strongly advised to not have the close method throw InterruptedException. This exception interacts with a thread's interrupted status, and runtime misbehavior is likely to occur if an InterruptedException is suppressed. More generally, if it would cause problems for an exception to be suppressed, the AutoCloseable.close method should not throw it.