RFR: 8279283 - BufferedInputStream should override transferTo [v7]

[Daniel Fuchs]

On Tue, 6 Sep 2022 07:55:55 GMT, Markus KARG <duke at openjdk.org> wrote:

>> test/lib/jdk/test/lib/hexdump/HexPrinter.java line 1194:
>> 
>>> 1192:             byteOffset += size;
>>> 1193:             return size;
>>> 1194:         }
>> 
>> This is an indication that overriding `transferTo()` in `BufferedInputStream` has potential compatibility impacts on its subclasses. Therefore I would suggest adding a check in `BufferedInputStream::transferTo` to only override the behavior if not subclassed. Something like:
>> 
>> 
>>      if (this.getClass() != BufferedInputStream.class) {
>>          // a custom subclass may have expectations on which
>>          // methods tansferTo() will call. Revert to super class
>>          // behavior for compatibility.
>>          return super.transferTo(out);
>>      }
>>      ... otherwise proceed with the new implementation ...
>> 
>> 
>> Then you can revert the changes in this test.
>
> I do not see that it makes much sense to add such a safety means before I am finished with the inspection of the subclasses. As you can see in this example, it was pretty easy to fix it, and there are only few such subclasses at all. So instead of preparing against possibly non-existing easyt-to-fix problems, I prefer fixing the actual problems. They can only occur inside of the JDK itself, as relying on any particular implementation inside of the JDK by non-JDK classes simply would be a complete programming fault and *must* get fixed.

It's a well known behavior that overriding the`read(...)` method is sufficient to implement subclass behavior. This will no longer be the case if `transferTo(...)` no longer calls `this.read(...)` as it used to. There are many undocumented behaviors that have been depended on over the year - and though I agree with the sentiment that it's not a good idea to depend on undocumented behavior, this is one that is long standing, and mostly legitimate, especially for those classes that were coded before `transferTo` was added to `InputStream`.

The issue here is that things would mostly work for those custom subclassed, up to the point where some unsuspecting code might call `transferTo`. This could lead to bugs that might become hard to diagnose, and of course would only be seen at runtime.
As a point of comparison, look at what [JEP 425](https://openjdk.org/jeps/425) did to change the locking in `BufferedInputStream` (see how the lock object is used/initialized).

How many classes of `BufferedInputStream` are there in the wild, outside of the JDK?
How many of them have custom behavior implemented in `read`, that would be broken if `transferTo` no longer called `this.read`?

If you're changing an observable behavior, you will need a CSR, you will need to evaluate the compatibility risk and justify it, you will need to write a Release Note to warn about the compatibility risk, so that custom implementations that have such a dependency can be changed to also override `transferTo`.

The main question is whether the advantage of changing the behavior outweigh the risk of regression it might cause in subclasses. Such might be the case - or not.
The middle ground - and safer path - is to override the behaviour only for the base class and those subclasses that do not depend on the default behavior as was done by JEP 425...

-------------

PR: https://git.openjdk.org/jdk/pull/6935