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 <vtewa...@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.mim...@gmail.com> > Sent by: "core-libs-dev" <core-libs-dev-boun...@openjdk.java.net> > To: Vyom Tiwari <vyomm...@gmail.com> > Cc: core-libs-dev <core-libs-dev@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 <vyomm...@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.mim...@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.ra...@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.mim...@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
