RFR: 8279283 - BufferedInputStream should override transferTo [v7]

[Alan Bateman]

On Tue, 6 Sep 2022 12:10:00 GMT, Daniel Fuchs <dfuchs at 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...
>> 
>> 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.

BIS dates from JDK 1.0 and it's not hard to find examples that extend it. The HexPrinter test reminds us that it has been possible for 25+ years to override the read methods and snoop on all bytes that are read. Adding an override of the transferTo method that bypasses the read methods might break these subclasses. So I think Daniel is right that we have to be cautious.

So I think the feedback that you have now is that bypassing the buffer is problematic when using mark, when there are buffered bytes, or when BIS has been extended. It may be that the starting point is something very conservative like this:


    @Override
    public long transferTo(OutputStream out) throws IOException {
        if (getClass() == BufferedInputStream.class
                && ((count - pos) <= 0) && (markpos == -1)) {
            return getInIfOpen().transferTo(out);
        } else {
            return super.transferTo(out);
        }
    }


There is also the issue of locking and async close to look into.

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

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