On Tue, 6 Sep 2022 11:26:18 GMT, Markus KARG <d...@openjdk.org> wrote:
>> 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... > >> 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... > > I understand your claim but disagree with the conclusion. You could impose it > to *any* behavior change of *any* class -- this is no special case here. I do > not see that we need a CSR as I do not change the *documented* behavior > (neither the JavaDoc nor the Spec). Also I need to ask back: Do you *really* > assume there are such lots of people using `transferTo` with failing > subclasses of BIS? I doubt that, actually. You try to fix things possible not > broken at all. We should concentrate on facts, not on assumptions, IMHO. > > BTW, even *if* we would add the safety net you proposed, the existing code > would be wrong *still*, so we should *not* undo the fixed test, and we still > should fix the JDK-internal BIS subclasses. It is no good idea to keep the > bad code smell for longer, just because there is a safety net in the > superclass. One fine day that safety net will break, and nobody would > remember this PR discussion... > > Having said that, I leave it up to @AlanBateman to decide, as I have no > strong feelings about the issue you raised. With the changes you proposed a CSR will definitely be needed. ------------- PR: https://git.openjdk.org/jdk/pull/6935
