On Mon, 7 Oct 2024 17:39:33 GMT, Markus KARG <d...@openjdk.org> wrote:
>>> keep the reference to the char sequence "forever"
>>
>> Note that this only happens if the `Reader` instance is kept forever; but a
>> `Reader` is supposed to be used and closed and discarded instead of being
>> held indefinitely after closing. If users are bad enough, they can never
>> call `close` and the non-final `cs` is still leaked.
>>
>> The right way of GC freeing is to let the GC free the reader and the char
>> sequence together, instead of allowing to free when a client keeps this
>> reader instance erroneously after closing.
>
> I really do see you point, nevertheless I think we should be kind to "bad"
> users, too. In the end, there is *no* specification which explicitly tells
> "bad" users that readers are support to be short-living (despite it being
> rather obvious), while `close` explicitly mentions freeing *all* resources,
> so it is rather unexpected to keep something past `close`, even for short
> term. NB: `StringReader` applies the very same behavior, too (using `s` and
> `str` variables), that's where I copied the code from originally. It is not a
> big problem for us to have two variables, neither that the name of the
> variable is `source` (in fact, I do like `source` even more than `cs`, but
> this is just my personal preference). To sum up, I really dislike the idea to
> keep the reference for any longer than essentially needed. If it's just about
> the name of the variable, I could simply switch them if you prefer that.
On a linux x64 build:
./javap -p java.io.Reader$2
Compiled from "Reader.java"
class java.io.Reader$2 extends java.io.Reader {
private final int length;
private java.lang.CharSequence cs;
private int next;
private int mark;
final java.lang.CharSequence val$source;
java.io.Reader$2(java.lang.CharSequence);
private void ensureOpen() throws java.io.IOException;
public int read() throws java.io.IOException;
public int read(char[], int, int) throws java.io.IOException;
public long skip(long) throws java.io.IOException;
public boolean ready() throws java.io.IOException;
public boolean markSupported();
public void mark(int) throws java.io.IOException;
public void reset() throws java.io.IOException;
public void close();
}
The javap in the built binaries of this patch shows that this anonymous class
already keeps the `CharSequence val$source` in a final field; therefore, your
setting the `cs` field to `null` will not help GC or anything of that sort.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21371#discussion_r1790782348
