On Sat, 9 Oct 2021 15:49:01 GMT, liach <d...@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
