Hi Michael,
I'm not a reviewer. I just checked the webrev and saw this change:
--- old/src/java.base/share/classes/java/net/AbstractPlainSocketImpl.java
2018-06-06 08:34:38.000000000 +0100
+++ new/src/java.base/share/classes/java/net/AbstractPlainSocketImpl.java
2018-06-06 08:34:38.000000000 +0100
@@ -37,6 +37,7 @@
...
}
} catch (IOException e) {
- close();
- throw e;
+ throw SocketExceptions.of(e, new InetSocketAddress(address, port));
Is it intentional to remove that call to close()?
The rest of the changes look fine to me.
-Jaikiran
On 06/06/18 1:15 PM, Michael McMahon wrote:
Hi all,
Finally to return to this topic. We have looked at a few different
approaches
and it seems the best way is to define a security property that can be set
in the java.security configuration file, but which can be overridden as a
system property. The current behavior will remain the default, but setting
the property will add addressing information to exception texts.
The change applies to all TCP socket types in java.net and java.nio.
Webrev at:
http://cr.openjdk.java.net/~michaelm/8204233/webrev.1/index.html
Thanks,
Michael.
On 01/05/2018, 09:48, Michael McMahon wrote:
Peter,
Just to followup on this. We are still investigating a few options
for doing this
and it might be a few more weeks before we get a decision. I did take
your patch
as a starting point, and modified it to also work with NIO, and also
to preserve
the original exception (with original stack trace) which I think is
desirable.
I don't think there is much point in reviewing the webrev until we
get the decision
mentioned above. But, we should be able to push it soon after that.
Thanks,
Michael
On 23/04/2018, 10:05, Péter Gergely Horváth wrote:
Hi Tobias,
Thank you for pointing me to that thread: it's good to have that
context (it was sent before I joined the mailing list, so please
bear with me).
I understand the JDK developers want to be safe than sorry around
reporting target addresses and I absolutely agree with that point.
However considering how useful it would be to have this _optionally_
for debugging, I am wondering if it would be possible to have a
dedicated Java system property defined for this (e.g.
'java.net.socket.reportAddressInException' or something like that),
which would enable this behaviour (retaining the current behaviour
of *not reporting anything by default.*).
What do you think about this, guys? With this in place both the
secure-by-default requirement would be met, and there would be a
powerful tool available to fight the horrors of debugging a running
complex distributed application from its logs.
Thanks,
Peter
On Sun, Apr 22, 2018 at 10:21 PM, James Roper <ja...@lightbend.com
<mailto:ja...@lightbend.com>>wrote:
This would be especially useful in asynchronous applications -
since in those cases the exception rarely maps back to a place
in user code that would indicate what is being connected to. As
someone who has spent a lot of time supporting developers who
use asynchronous libraries and post exceptions of this nature
(supporting both in open source, eg on stack overflow, as well
as providing commercial support), where I don't have access to
their code base so I can't do the necessary investigations
directly myself, having the attempted address and port in the
error message would save a lot of time, and probably even
prevent a lot of people from requiring support in the first place.
On 22 April 2018 at 20:59, Péter Gergely Horváth
<peter.gergely.horv...@gmail.com
<mailto:peter.gergely.horv...@gmail.com>>wrote:
Hi All,
I am wondering if it would be possible to make a minor
improvement to the way *java.net.Socket* reports
connectivity errors and incorporate the attempted address,
port and the timeout used into the exception message.
The current implementation emits a generic error message,
which is not that helpful when one is operating a _large_
application. (Consider e.g. Big Data or complex legacy
systems written by someone else).
java.net.ConnectException: Connection refused (Connection
refused)
at java.net.PlainSocketImpl.socketConnect(Native Method)
at java.net
<http://java.net>.AbstractPlainSocketImpl.doConnect(AbstractPlainSocketImpl.java:350)
at java.net
<http://java.net>.AbstractPlainSocketImpl.connectToAddress(AbstractPlainSocketImpl.java:206)
at java.net
<http://java.net>.AbstractPlainSocketImpl.connect(AbstractPlainSocketImpl.java:188)
at java.net.SocksSocketImpl.connect(SocksSocketImpl.java:392)
at java.net.Socket.connect(Socket.java:589)
at java.net.Socket.connect(Socket.java:538)
at java.net.Socket.<init>(Socket.java:434)
at java.net.Socket.<init>(Socket.java:211)
at Sample.main(Sample.java:9)
I have looked into the JDK code base and implemented a patch
that reports the address, port and timeout used in the error
message without touching any native parts (see attached
patch file). I have tested this (created my own build of the
JDK and run a sample application against it) and it seems to
work properly.
Would it be possible to incorporate this change into the
official JDK code base? I do believe it would help a lot of
people out there.
Based on my understanding, once I have signed the OCA, I
should simply write an email to the group and request
a sponsor to pick up this issue. Could someone help me with
this?
Thank you,
Peter
--
*James Roper*
/Senior Octonaut/
Lightbend <https://www.lightbend.com/> – Build reactive apps!
Twitter: @jroper <https://twitter.com/jroper>
