java.net.Socket should report the attempted address and port
Michael McMahon
michael.x.mcmahon at oracle.com
Fri Jun 8 07:00:56 UTC 2018
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 <james at lightbend.com
>>>>> <mailto:james at 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.horvath at gmail.com
>>>>> <mailto:peter.gergely.horvath at 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>
>>>>>
>>>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20180608/d4a0d42f/attachment-0001.html>
More information about the net-dev
mailing list