[PATCH] JDK-8228580 DnsClient TCP socket timeout

[Milan Mimica]

Hi

I agree that makes it more simple, but doing it this way ensures more
precise timeout, subtracting the remaining time from global timeout
value. I still rely on read() to throw SocketTimeoutException. The
solution you suggest would potentially make each read() operation wait
almost up to global timoeut value, totaling in a much larger execution
time from user's perspective.
I was following the pattern how it was done for UDP requests. It uses
the same logic.

On Tue, 27 Aug 2019 at 10:52, Vyom Tewari26 <vtewar26 at in.ibm.com> wrote:
>
> Hi Milan,
>
> Thanks for attaching the .dns file, I gone through the changes and  it looks like you don't need "readWithTimeout" method to manipulate timeout  and throw  SocketTimeoutException explicitly.
>
> ###################################
> if (timeoutLeft <= 0)
> +            throw new SocketTimeoutException();
> ###################################
>
> Once you set the timeout on socket( sock.setSoTimeout(timeout)), the  a read() call on the InputStream associated with  this Socket will block for only this amount of time.  If the timeout  expires, a java.net.SocketTimeoutException will be thrown.
>
> My suggestion is to just set the timeout on socket and remove rest of the additional changes in inner 'Tcp' class.
>
> Thanks,
> Vyom
>
>
> ----- Original message -----
> From: Milan Mimica <milan.mimica at gmail.com>
> Sent by: "core-libs-dev" <core-libs-dev-bounces at openjdk.java.net>
> To: Vyom Tiwari <vyommani at gmail.com>
> Cc: core-libs-dev <core-libs-dev at openjdk.java.net>
> Subject: [EXTERNAL] Re: [PATCH] JDK-8228580 DnsClient TCP socket timeout
> Date: Fri, Aug 23, 2019 1:17 AM
>
> Hi
>
> Indeed I have forgotten. Re-attached, with aesthetic fixes to the test.
>
> On Thu, 22 Aug 2019 at 05:38, Vyom Tiwari <vyommani at gmail.com> wrote:
> >
> > Hi Milan,
> >
> > Your test need the corresponding "TcpTimeout.dns" file to run successfully, I believe you forgot to add with your patch.
> > please check the existing tests in the same folder if you need any additional information.
> >
> > Thanks,
> > Vyom
> >
> > On Wed, Aug 21, 2019 at 10:48 PM Milan Mimica <milan.mimica at gmail.com> wrote:
> >>
> >> Hi Pavel
> >>
> >> Updated the patch with the jtreg test.
> >> The test hangs when the fix is not applied. I don't know why
> >> main/timeout=20 does not work for me.
> >>
> >> On Tue, 20 Aug 2019 at 00:08, Pavel Rappo <pavel.rappo at oracle.com> wrote:
> >> >
> >> > Thanks for doing that. I've only skimmed through the patch and I’d recommend that no matter if the changes are adequate or not there should be a test [1]:
> >> >
> >> > "...A unit test, written for the jtreg test harness, that validates your change. The test should fail when run against a build without the change and succeed when run against a build with the change. Unit tests aren't always practical, especially for changes such as performance improvements or fixes to bugs that are highly platform-dependent, but if practical then a unit test is required."
> >> >
> >> > Please consider adding it to the patch while the changes are being (preliminary) reviewed.
> >> >
> >> > -Pavel
> >> >
> >> > -------------------------------------------------------------------------------
> >> > [1] https://openjdk.java.net/contribute/
> >> >
> >> > > On 19 Aug 2019, at 17:20, Milan Mimica <milan.mimica at gmail.com> wrote:
> >> > >
> >> > > Hello list
> >> > >
> >> > > Please find the attached patch that fixes JDK-8228580.
> >> > >
> >> > > It works the similar way UDP timeout does: calculate the initial
> >> > > timeout from retry attempt, and account for duration of every blocking
> >> > > call on the TCP socket.
> >> > >
> >> > > I am listed as Author[1] on "jdk" project.
> >> > >
> >> > > [1] https://openjdk.java.net/census#mmimica
> >> > >
> >> > >
> >> > > --
> >> > > Milan Mimica
> >> > > <JDK-8228580.patch>
> >> >
> >>
> >>
> >> --
> >> Milan Mimica
> >
> >
> >
> > --
> > Thanks,
> > Vyom
>
>
>
> --
> Milan Mimica
>
>
>
>


-- 
Milan Mimica