On Friday 12 October 2018 04:31 PM, Chris Hegarty wrote:
Daniel,
On 12/10/18 11:26, Daniel Fuchs wrote:
Hi,
Maybe a more conservative implementation could have been:
int available = impl.available();
return eof ? 0 : available;
That buys us little more than we had prior to this change,
since impl.available will still call into native before
checking the EOF status.
If we want to keep this, then we need:
public int available() throws IOException {
if (impl.isClosedOrPending()) {
throw new SocketException("Socket closed");
}
if (eof) {
return 0;
} else {
return impl.available();
}
}
Above change is working as expected, i modified the "CloseAvailable"
test little bit and with modified code, SocketInputstream.available() is
now throwing "java.net.SocketException: Socket closed" exception if the
underline socket is closed.
I will prepare a patch along with modified test and send it for review.
I ran our internal tests and it looks like it was not caught there.
Thanks,
Vyom
I almost suggested that yesterday, but I saw that
read() already had a logic similar to what Vyom was
proposing for available:
146 // EOF already encountered
147 if (eof) {
148 return -1;
149 }
which AFAICT means that read returns -1 instead of throwing
if the socket is closed and EOF was previously reached.
That is correct. While not intuitive, I don't propose
that we change this. ( if this were a new implementation
then I think it should throw IOE for this scenario, but
we are where we are ).
-Chris.
best regards,
-- daniel
On 12/10/2018 09:55, Chris Hegarty wrote:
On 12/10/18 08:29, Alan Bateman wrote:
On 11/10/2018 09:03, vyom tewari wrote:
Hi Chris,
Thanks for review, please find the updated
webrev(http://cr.openjdk.java.net/~vtewari/8189366/webrev0.1/index.html)
where i included the test.
Can you explain the behavior change for the closed socket case?
Will this change mean that available returns 0 when it previously
throw IOException?
You are correct. This is not intentional, or desirable.
We should revert the change or add an additional check
for isClosedOrPending. Since this is already pushed, I
filed a new JIRA issue to track this.
https://bugs.openjdk.java.net/browse/JDK-8212114
-Chris.
