Regression in EPollArrayWrapper causes NPE when fd > 64 * 1024

[Norman Maurer]

Hi Alan,

just a tiny thing but why not check if force is true before try to access the Map (like in my proposed patches)? Not sure it gives much gain in terms of performance but it can't harm at all… 

About the test-case, I think it's important to have a test cover the usage of the eventsHigh Map. Otherwise it's just "too easy" to break things and it may be take some time to get noticed as only "a few" users are affected. I think you could even "adjust" the MAX_UPDATE_ARRAY_SIZE with reflection if you not want to expose it.  

WDYT ?

---
Norman Maurer
nmaurer at redhat.com

JBoss, by Red Hat



Am 17.09.2013 um 12:10 schrieb Alan Bateman <Alan.Bateman at oracle.com>:

> On 16/09/2013 21:02, Norman Maurer wrote:
>> 
>> Hi Alan,
>> 
>> for sure I want to contribute a patch. Seems like the patches / reproducer are visible in the mail archive[1]. 
>> 
>> Anyway here are the files included inline:
> Attachments are usually stripped by the OpenJDK mail servers so I'm surprised they are there.
> 
> On the test case (or reproducer): as there is configuration required to allow >64k connections then a simpler way to demonstrate it is to initially consume 64k file descriptors (by opening files or create SocketChannels). That's what I did when I saw your mail in order to duplicate it quickly. We need to decide whether to attempt to include a test case with this patch. I'm in two minds on this as I don't know how often the test will be run in environments where the maximum number of file descriptors is unlimited or high enough. An alternative (for a future patch perhaps) is to make MAX_UPDATE_ARRAY_SIZE configurable so that the spilling can tested without requiring the maximum number of file descriptors to be increased.
> 
> Your patch is essentially the same as what I tested with yesterday. I've changed to the attached (to make it consistent with the eventsLow code mostly). If you are okay with this then I will push it listing you as contributor.
> 
> -Alan.
> 
> 
> diff --git a/src/solaris/classes/sun/nio/ch/EPollArrayWrapper.java b/src/solaris/classes/sun/nio/ch/EPollArrayWrapper.java
> --- a/src/solaris/classes/sun/nio/ch/EPollArrayWrapper.java
> +++ b/src/solaris/classes/sun/nio/ch/EPollArrayWrapper.java
> @@ -164,6 +164,16 @@
>      }
>  
>      /**
> +     * Returns {@code true} if updates for the given key (file
> +     * descriptor) are killed.
> +     */
> +    private boolean isEventsHighKilled(Integer key) {
> +        assert key >= MAX_UPDATE_ARRAY_SIZE;
> +        Byte value = eventsHigh.get(key);
> +        return (value != null && value == KILLED);
> +    }
> +
> +    /**
>       * Sets the pending update events for the given file descriptor. This
>       * method has no effect if the update events is already set to KILLED,
>       * unless {@code force} is {@code true}.
> @@ -175,7 +185,7 @@
>              }
>          } else {
>              Integer key = Integer.valueOf(fd);
> -            if ((eventsHigh.get(key) != KILLED) || force) {
> +            if (!isEventsHighKilled(key) || force) {
>                  eventsHigh.put(key, Byte.valueOf(events));
>              }
>          }
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/nio-dev/attachments/20130917/503c764a/attachment.html