Hi Seán,
this looks very good.
I've got a few minor points:
1. In line 65 where you catch a possible unexpected exception, you could also
mention the thread name in the output.
2. I would check for failure condition at the end of each test loop(line 91,
for (NetworkInterface ni : toTest)... ) after a particular interface was tested
and in the case of failed==true, terminate the loop. If we see an error with
one interface, it's probably not necessary to test others.
3. line 100,101:
while (!executor.isTerminated()) {
}
This looks a bit odd. Maybe you should call executor.awaitTermination() inside
the loop? Or you could as well use executor.invokeAll() which will return only
after all threads are done.
4. line 96:
new GetMacAddress(ni, ni.getName() +"-Thread-" + i);
There's a space missing between + and "-Thread-".
Best regards
Christoph
> -----Original Message-----
> From: Seán Coffey [mailto:sean.cof...@oracle.com]
> Sent: Freitag, 23. Juni 2017 11:57
> To: Langer, Christoph <christoph.lan...@sap.com>
> Cc: Vyom Tewari <vyom.tew...@oracle.com>; net-dev <net-
> d...@openjdk.java.net>; Mark Sheppard <mark.shepp...@oracle.com>
> Subject: Re: RFR : 8182672: Java 8u121 on Linux intermittently returns null
> for
> MAC address
>
> Thanks to Christoph, Vyom and Mark for the reviews.
>
> I've improved the testcase as per feedback. Hope this meets all requests :
>
> http://cr.openjdk.java.net/~coffeys/webrev.8182672.jdk10.v2/webrev/
>
> Regards,
> Sean.
>
> On 22/06/17 22:36, Langer, Christoph wrote:
> > Hi Sean,
> >
> > I played with it a bit more and now really understand Vyoms observation.
> So, what he sees is not the original concurrency issue but he encounters a
> SocketException on some interface, where this is supposed to occur upon
> calling getHardwareAddress().
> >
> > So, to enable the testcase to run robustly on any platform, any hardware
> and any network configuration, I suggest to modify the test like this:
> >
> > 1. In main, enumerate all interfaces once
> > 2. Iterate over all interfaces, exlude loopback and see if a single call to
> getHardwareAddress() won't yield null or an Exception.
> > 3. For each interface where a valid mac address could be obtained once,
> start the executor threads to stress getHardwareAddress() in parallel, e.g.
> like 5 threads doing it 100 times in parallel. I would also suggest to use per
> thread counters instead of one global 'count' as we have right now.
> >
> > Furthermore, the test output could be improved a bit, e.g. when it comes
> to an exception, the thread name could be mentioned, too.
> >
> > Best regards
> > Christoph
> >
> >> -----Original Message-----
> >> From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of
> >> Seán Coffey
> >> Sent: Donnerstag, 22. Juni 2017 20:17
> >> To: Vyom Tewari <vyom.tew...@oracle.com>; net-dev <net-
> >> d...@openjdk.java.net>
> >> Subject: Re: RFR : 8182672: Java 8u121 on Linux intermittently returns null
> for
> >> MAC address
> >>
> >> Hi Vyom,
> >>
> >> thanks for testing. Null is a valid value for some interfaces. I've
> >> excluded the loopback interface from being testing. Perhaps there's
> >> other issues at play here. We know that getHardwareAddress() can
> throw
> >> SocketException if I/O fails. For this particular scenario we're not
> >> interested in that and perhaps that can be ignored.
> >>
> >> I'll take another look.
> >>
> >> regards,
> >> Sean.
> >>
> >> On 22/06/2017 18:50, Vyom Tewari wrote:
> >>> Hi Sean,
> >>>
> >>> with your patch as well your test case is failing on my
> >>> laptop(Ubuntu16.04), when i tried to run on jdk8 i am getting below
> >>> error.
> >>>
> >>> java.net.SocketException: No such device (ioctl(SIOCGIFHWADDR)
> failed)
> >>> at java.net.NetworkInterface.getMacAddr0(Native Method)
> >>> at
> >>>
> >>
> java.net.NetworkInterface.getHardwareAddress(NetworkInterface.java:457
> >> )
> >>> at com.java.test.GetMacAddress.run(GetMacAddress.java:66)
> >>> at
> >>>
> >>
> java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.jav
> >> a:1142)
> >>> at
> >>>
> >>
> java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.ja
> >> va:617)
> >>> at java.lang.Thread.run(Thread.java:745)
> >>> java.net.SocketException: No such device (ioctl(SIOCGIFHWADDR)
> failed)
> >>> at java.net.NetworkInterface.getMacAddr0(Native Method)
> >>> at
> >>>
> >>
> java.net.NetworkInterface.getHardwareAddress(NetworkInterface.java:457
> >> )
> >>> at com.java.test.GetMacAddress.run(GetMacAddress.java:66)
> >>> at
> >>>
> >>
> java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.jav
> >> a:1142)
> >>> at
> >>>
> >>
> java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.ja
> >> va:617)
> >>> at java.lang.Thread.run(Thread.java:745)
> >>> Exception in thread "main" java.lang.RuntimeException: Failed
> >>> at com.java.test.GetMacAddress.main(GetMacAddress.java:96)
> >>> mac id is null for interface cscotun0- Thread0
> >>> Testing: cscotun0
> >>> mac id is null for interface cscotun0- Thread3
> >>> Testing: cscotun0
> >>>
> >>> Thanks,
> >>>
> >>> Vyom
> >>>
> >>>
> >>> On Thursday 22 June 2017 09:59 PM, Seán Coffey wrote:
> >>>> JDK 10 fix required to correct a race issue in NetworkInterface. I
> >>>> don't believe the ifreq struct needs to be static in any case. New
> >>>> auto unit testcase also. I propose to skip this fix for JDK 9 and fix
> >>>> in an update release for that family. I also plan to port this to
> >>>> jdk8u-dev.
> >>>>
> >>>> https://bugs.openjdk.java.net/browse/JDK-8182672
> >>>> webrev :
> >>>> http://cr.openjdk.java.net/~coffeys/webrev.8182672.jdk10/webrev/
> >>>>
> >>>> regards,
> >>>> Sean.
