Re: RFR: 8189366: SocketInputStream.available() should check for eof
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? -Alan
Re: RFR: 8189366: SocketInputStream.available() should check for eof
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.
Re: [12] RFR 8187522: sun/net/ftp/FtpURLConnectionLeak.Java timed out
On 12/10/18 03:46, Chris Yin wrote: ... Thanks for your suggestion, currently, I just added a comment to explain the 220 response message and sent updated webrev in earlier thread, I may prefer to process in that way for now if no objection since separate specific test added for 8151586 is already out of scope of current bug :). We may have another bug/enhancement to track it if necessary. Thanks Agreed. > http://cr.openjdk.java.net/~xyin/8187522/webrev.01/ This looks good to me. -Chris.
Re: RFR: 8189366: SocketInputStream.available() should check for eof
Hi,
Maybe a more conservative implementation could have been:
int available = impl.available();
return eof ? 0 : available;
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.
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.
Re: RFR: 8189366: SocketInputStream.available() should check for eof
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();
}
}
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.
Re: RFR: 8189366: SocketInputStream.available() should check for eof
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.
Re: RFR: 8189366: SocketInputStream.available() should check for eof
On 12/10/2018 12:01, Chris Hegarty wrote:
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();
}
}
Hmmm...
I thought impl.available() was going to throw the
exception?
If you don't want to penalize the regular case where
eof is false, and the impl is supposed to throw the
exception, and you want to avoid to go back to the
native impl when eof has been reached, then maybe you
need:
public int available() throws IOException {
if (eof) {
if (impl.isClosedOrPending()) {
throw new SocketException("Socket closed");
}
return 0;
} else {
return impl.available();
}
}
and that's assuming the that the exception that the impl would
throw is exactly new SocketException("Socket closed");
Can't help feeling that
int available = impl.available();
return eof ? 0 : available;
addresses the issue of available potentially returning garbage
after EOF while being much less risky...
cheers,
-- daniel
Re: RFR: 8189366: SocketInputStream.available() should check for eof
On Fri, Oct 12, 2018 at 6:01 AM Chris Hegarty wrote: > 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 ). I am glad then that it is not a new implementation. Returning an EOF signal is a very reasonable behavior for a closed input stream or input channel. In most cases, it is more difficult for I/O handling code to distinguish the two states (open+eof versus closed), and in every case I've ever known, they are handled in the same way, or intended to be. There is no useful way I know of to exploit a side effect of exception throwing in the read-on-closed case. Also, I think it's a lot more reasonable for programs to expect that available() will return 0 when the stream is closed than for available() to be, in effect, an openness check. Granted that available() is not really useful anyway, but were one to use it, having an extra failure state to deal with probably isn't helpful. -- - DML
Re: RFR: 8189366: SocketInputStream.available() should check for eof
On 12/10/2018 14:43, David Lloyd wrote: On Fri, Oct 12, 2018 at 6:01 AM Chris Hegarty wrote: 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 ). I am glad then that it is not a new implementation. Returning an EOF signal is a very reasonable behavior for a closed input stream or input channel. It's an interesting discussion point as it will likely depend on the protocol as to how premature EOF is handled. Also if you asynchronously close the socket then it will be random as to whether a reader on the same socket will read -1 or throw an exception. -Alan
