RFR: 8284638: store skip buffers in InputStream Object

[XenoAmess]

On Sat, 9 Oct 2021 15:49:01 GMT, liach <duke at openjdk.java.net> wrote:

> You should really post your changes to jdk-dev at openjdk.java.net before submitting these so you can know if a change is potentially good or bad. Since jdk pull requests require issue numbers, you can ask people on the list if this is a good idea; if it is, someone on the list may create an issue for you, which probably means your suggestion is valid, and only then you should send a pull request.

you mean I should pin the patch file at mailing list first? Get it.

I just think it easier to view the code changes at github pr page than the patch file...but if the workflow should be asking at the mailing list first, then I will obey.

> src/java.base/share/classes/java/io/InputStream.java line 550:
> 
>> 548: 
>> 549:         if ((skipBuffer == null) || (skipBuffer.length < size)) {
>> 550:             skipBuffer = new byte[size];
> 
> In the `Reader` class, `skipBuffer` access is thread-safe as it's behind a synchronization on the `lock`. This one isn't and is subject to weird java memory models: in extreme cases, if the `skipBuffer` is set to non-null on another thread, it may be non-null on first field read and null on second field read.
> 
> Hence you should write code like this instead:
> 
>         byte[] skipBuffer = this.skipBuffer;
>         if ((skipBuffer == null) || (skipBuffer.length < size)) {
>             this.skipBuffer = skipBuffer = new byte[size];
>         }
> 
> so you only read the field once and get consistent results.

> In the `Reader` class, `skipBuffer` access is thread-safe as it's behind a synchronization on the `lock`. This one isn't and is subject to weird java memory models: in extreme cases, if the `skipBuffer` is set to non-null on another thread, it may be non-null on first field read and null on second field read.
> 
> Hence you should write code like this instead:
> 
> ```java
>         byte[] skipBuffer = this.skipBuffer;
>         if ((skipBuffer == null) || (skipBuffer.length < size)) {
>             this.skipBuffer = skipBuffer = new byte[size];
>         }
> ```
> 
> so you only read the field once and get consistent results.

@liach

sorry for missing considering about multi-thread situations...
...the question you bring are correct, but maybe this is not that easy to solve.


        byte[] skipBuffer = this.skipBuffer;
        if ((skipBuffer == null) || (skipBuffer.length < size)) {
            this.skipBuffer = skipBuffer = new byte[size];
        }


this solution might also bring another problem:

first, this.skipBuffer = byte[3]

then, Thread A call skip(10)

meanwhile Thread B call skip(6)

in extream situation, when doing `this.skipBuffer = skipBuffer` in Thread B, it might make this.skipBuffer to a byte[6] in thread A, and thus cause a IndexOutofBoundException in Thread A.

So only way to do it safely seems adding some lock like in Reader... that is pretty costy, I don't think it worth...

closed.

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

PR: https://git.openjdk.java.net/jdk/pull/5872