2

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?

greenButMellow
  • 316
  • 1
  • 9
  • 6
    If the only method existing for closing a resource fails, what *can* you do? Nothing, obviously. You already did the best you can. Besides, I don’t understand why you call “try-with-resources” “optimistic”; it’s as defensive as Java code can be. – Holger Feb 07 '23 at 11:47
  • 3
    Assume you had some mechanism to re-try the close if the auto-close fails. What would you do if that fails? How many layers do you want to add? What would happen if the close fails but doesn't report it as an exception and just silently leaks resources? Some kinds of problems you simply can't code around ... – Joachim Sauer Feb 07 '23 at 11:48
  • 1
    Also you can never say "yes" to this unless you state that the answer covers your own code only. – Newerth Feb 07 '23 at 11:48
  • 4
    Generally: try-with-resource is not automagically worse or better than manual code, but it sure as hell is better than at least 95% of all hand-written closing code I've seen in the wild. – Joachim Sauer Feb 07 '23 at 11:49
  • 3
    "Is it correct to say that this code is prone to a resource/memory leak?" - try-with-resources was actually introduced to reduce the risk of a memory leak as developers might forget to close the resource in a traditional try-finally. And as Holger said, what would you do if closing fails? – Thomas Feb 07 '23 at 11:49
  • Thanks for the answers! I think I understand the spirit of the answer. Should I delete the question though? – greenButMellow Feb 07 '23 at 11:50
  • @JoachimSauer, can you elaborate on that? – greenButMellow Feb 07 '23 at 11:51
  • 1
    What he means is: try-with-resources was introduced because people implemented all sorts of things using try/catch/finally ... and got that wrong. try-with-resources eliminates many things you can get wrong when implementing such logic manually. – GhostCat Feb 07 '23 at 11:53
  • 3
    @greenButMellow: try-with-resources can be re-implemented using the traditional try-catch-finally code, but it'll look quite wordy, due to the many possible cases it handles (including exceptions during the close). Most "manual" code that does the quivalent won't handle things like that. For example, if an exception happens *inside* the `try` and *another one* happens in the `close`. try-with-resources will report both of those (one as supressed). – Joachim Sauer Feb 07 '23 at 11:53
  • @JoachimSauer, thanks! Can you refer me to the source code of try-with-resources? – greenButMellow Feb 07 '23 at 11:58
  • 2
    @greenButMellow: [the JLS gives the equivalent code](https://docs.oracle.com/javase/specs/jls/se17/html/jls-14.html#jls-14.20.3.1). [This SO question](https://stackoverflow.com/questions/23611940/) might be more readable, though. – Joachim Sauer Feb 07 '23 at 12:00

2 Answers2

3

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.
rzwitserloot
  • 85,357
  • 5
  • 51
  • 72
1

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.

TopchetoEU
  • 697
  • 5
  • 9
  • 2
    This is an incorrect, or at least extremely badly stated answer. Specifically, _You are correct in your assumption that if an exception is thrown upon the freeing of transport, some memory leak will occur._ is just straight up false. There is nothing 'will' about it. In fact, your quoted docs state the exact opposite: It's a 'should not' case - implementors SHOULD NOT leak memory when closing/freeing is not fully possible, instead, they should free all resources and _then_ throw. The compiler/runtime cannot guarantee that implementors do it right... but then, – rzwitserloot Feb 07 '23 at 12:03
  • 2
    ... we're down to the academic case of 'well, ALL code COULD be buggy, so NO guarantee could EVER hold' which is stretching the concept far beyond any utility or meaning. Hence, misleading/incorrect. – rzwitserloot Feb 07 '23 at 12:03
  • 2
    Example: "Does `String x = expr();` mean that `x` can only point to a String or null, that it can NEVER POSSIBLY point to, say, an `InputStream`? With this line of thinking, the answer is: "No, it could point to an inputstream, if the JVM implementation is buggy / if you have a hacky classfile possibly due to a buggy `javac` and you turned the verifier off". Which is an answer so useless and misleading it's hard to tell the difference between that and 'outright false'. For that same reason, the answer to this question is: "NO", try-with does not hypothetically lead to memory leaks. – rzwitserloot Feb 07 '23 at 12:05