On Tue, 6 Sep 2022 07:55:55 GMT, Markus KARG <d...@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
