Review Request - JDK-6720349: (ch) Channels tests depending on hosts inside Sun (original) (raw)

Daniel Fuchs daniel.fuchs at oracle.com
Wed Nov 7 05:08:34 PST 2012

On 11/7/12 1:50 PM, Chris Hegarty wrote:

This looks really good Daniel.

Thanks.

* Trivially, RefusingServer. startNewServer L84 could use try-with-resources.

Refusing server is not connected - and thus not closeable. It's just a bean that wrap an address on which no one is listening. I could add a dummy noop close() for consistency but why bother?

* AbstractTcpServer.newServerSocket seems unnecessary

I naively thought I might be able to use a subclass of ServerSocket to trigger connection timeout with delays in the accept() method but it did not work. The method remained. Do you wish me to remove it? I was almost ready to push :-)

-- daniel

-Chris.

On 06/11/2012 16:10, Daniel Fuchs wrote: Hi,

Here is a new webrev incorporating Alan's feedback. I fixed the indenting issues by looking at the original code in the modified file (or in other files in the same directory), and trying to guess how the author might have indented the lines in question. That's just guesses on my part but hopefully it's better now than before. http://cr.openjdk.java.net/~dfuchs/JDK-6720349/webrev.01/ best regards, -- daniel

On 11/5/12 1:23 PM, Alan Bateman wrote: On 30/10/2012 18:32, Daniel Fuchs wrote: Hi,

I'm looking for reviewers for: JDK-6720349: (ch) Channels tests depending on hosts inside Sun http://cr.openjdk.java.net/~dfuchs/JDK-6720349/webrev.00/ This is to fix a test issue. Several tests in the jdknio test suites depends on remote services (echo, daytime, discard, to cite a few) being available on remote hosts inside Oracle network. The issue is that these tests cannot succeed outside of oracle network without modification, and were often known to fail even inside oracle networks depending on the availability of these hosts. This patch removes the dependencies on remote hosts by making the tests instantiate and start small TCP or UDP servers within the test VM, thus emulating the services that used to be provided by the internal remote hosts. There are however - a few side effects on the tests themselves (read: on what the tests are actually testing). Given that the servers accessed by the tests are now co-located on the same machine - some assumptions made by the test code had to be revisited. For instance, some of the tests assumed that by setting the non blocking flag of the channel to true, and then calling connect(), they would always obtain a channel in a connection pending state. With servers on the same hosts, this is no longer always the case (for instance on Solaris you end up with a channel already connected). As a consequence some of the tests, on some architectures, no longer exactly test what they used to be testing. I have identified these parts of the test code that may not always be activated and added comments to outline this. Tests performed: I ran the jdknio tests using JPRT (hotspotwest). best regards, -- daniel Thanks for doing this, this is long overdue. Also nice that there aren't too many changes to the tests themselves. My preference would be to put the test servers into a new class TestClass, to sit along aside TestThread and TestUtil. I suggest this because TestUtil was originally intended for utility methods for these tests. The only other general observation is that some of the indenting when statements or source lines slip into a second line is a bit inconsistent, I can't tell what is doing the formatting but it's just a bit inconsistent with the style that is used in this area (we don't have an JDK-wide coding convention/style guidelines). A couple of specific comments: - In Alias then the comment about Solaris should probably be replaced with a comment to say that the connection is established immediately. I also suggest dropping the message to System.err as this is inside the while loop and so will be printed LIMIT times. - BasicConnect.java (and many other tests) then the message that the connection is established immediately is printed to System.err, which suggests its an error or warning but it is normal so I'd suggest dropping it or printing it to System.out. - SocketChannel/Connect.java - the comment at L40-41 will likely confusing future maintainers, I don't think it is needed. - SocketChannel/ConnectState.java - I assume there isn't any need to check for exceptions==null or exceptions[0]==null in expectedExceptions. Otherwise the additionally handling of STPENDINGORCONNECT looks fine to me. So overall I think this will be great to get in. -Alan.

More information about the nio-dev mailing list