Jaikiran
Thanks for the review and the two comments. You're right the close() was
omitted
by mistake. On the initialization of the system property, I would think
that caching
in a static final is very common for such properties and I'm not aware
of that being
explicitly documented anywhere. But, I will check into that.
All the best,
Michael.
On 08/06/2018, 07:45, Jaikiran Pai wrote:
Also one other minor detail - given that this system property value is
read onceand cached as a static final, would it make sense to include
a note (in the property description of the java.security file?) that
explicitly states that this property needs to be set while launching
Java and can't be overridden programatically (since that can
potentially behave differentlydepending on when the SocketExceptions
class gets initialized)?
-Jaikiran
On 08/06/18 12:05 PM, Jaikiran Pai wrote:
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>
