harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Regis <xu.re...@gmail.com>
Subject Re: [jira] Commented: (HARMONY-6312) Concurrency problems in NIO
Date Fri, 11 Sep 2009 02:17:11 GMT
Regis wrote:
> Jesse Wilson (JIRA) wrote:
>>     [ 
>> https://issues.apache.org/jira/browse/HARMONY-6312?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12745221#action_12745221

>> ]
>> Jesse Wilson commented on HARMONY-6312:
>> ---------------------------------------
>>
>> The patch includes a test fails on the original code, but passes on 
>> the RI and on the fixed code.
>>
>> The patch makes some major surgery to SelectionKeyImpl. The 
>> fundamental change is that instead of adjusting readableFDs and 
>> writableFDs as the keys and interest ops change, this adjusts them 
>> before each select() operation. Select is already an O(N) operation 
>> (in terms of the number of keys), so constructing this array will not 
>> hurt performance. It also guarantees that the two arrays are "resized" 
>> the minimum number of times possible. The old code may grow the arrays 
>> more than necessary, only to shrink them before the native call.
>>
>> That file also received a small bit of documentation and style 
>> cleanup. Once this patch is live, I volunteer to submit a follow up 
>> that will improve variable names.
>>
>>
>>> Concurrency problems in NIO
>>> ---------------------------
>>>
>>>                 Key: HARMONY-6312
>>>                 URL: https://issues.apache.org/jira/browse/HARMONY-6312
>>>             Project: Harmony
>>>          Issue Type: Bug
>>>          Components: Classlib
>>>         Environment: SVN Revision: 801399
>>>            Reporter: Jesse Wilson
>>>         Attachments: NIO_Concurrency_issues.patch
>>>
>>>   Original Estimate: 72h
>>>  Remaining Estimate: 72h
>>>
>>> There's several potential concurrency problems in our NIO 
>>> implementation...
>>>  - Charset#isSupported isn't synchronized, but it accesses mutable 
>>> member cachedCharsetTable.
>>>  - In SelectionKeyImpl, stHash++ is a non-atomic operation so 
>>> multiple SelectionKey instances may receive the same hash code. (Why 
>>> not use the default hash code?)
>>>  - In SelectorImpl, the unmodifiableKeys member is never synchronized 
>>> on. But the documentation specifies that clients can synchronize on 
>>> that set to guard against changes
>>> These are the obvious problems; I suspect there are more subtle 
>>> concurrency defects at play here. I'll prepare a patch to address the 
>>> major issues, and we should consider a rigorous approach (Findbugs?) 
>>> to discover concurrency problems.
>>> #Android
>>
> 
> Jesse, thanks for this patch, I believe it make selector code easier 
> understand and maintain, but I still have concerns with iterating keySet 
> in prepareChannels.
> 
> The more registered channels, the slower prepareChannels, even more, we 
> have to travel the whole set more than once! Current implementation 
> maintains several mapping to avoid this. So do you have any data to 
> prove this patch is good as old one when large number of channels are 
> registered?
> 
> For arrays "resize" in prepareChannels, it can be simple avoid by patch 
> [1]. in [1] I added a new interface to accept count of readable/writable 
> FDs, so readableFDs and writableFDs don't need to be resized. After 
> patch [1] nothing to be done before select.
> 
> Index: 
> modules/luni/src/main/java/org/apache/harmony/luni/platform/INetworkSystem.java 
> 
> =====================================================================
> --- 
> modules/luni/src/main/java/org/apache/harmony/luni/platform/INetworkSystem.java 
> 
> +++ 
> modules/luni/src/main/java/org/apache/harmony/luni/platform/INetworkSystem.java 
> 
> @@ -157,6 +157,9 @@ public interface INetworkSystem {
>              FileDescriptor[] writeFDs, long timeout)
>              throws SocketException;
> 
> +    public int[] select(FileDescriptor[] readFDs,
> +            FileDescriptor[] writeFDs, int countRead, int countWrite, 
> long timeout)
> +            throws SocketException;
>      /*
>       * Query the IP stack for the local port to which this socket is 
> bound.
>       *
> Index: 
> modules/luni/src/main/java/org/apache/harmony/luni/platform/OSNetworkSystem.java 
> 
> =====================================================================
> --- 
> modules/luni/src/main/java/org/apache/harmony/luni/platform/OSNetworkSystem.java 
> 
> +++ 
> modules/luni/src/main/java/org/apache/harmony/luni/platform/OSNetworkSystem.java 
> 
> @@ -376,6 +376,31 @@ final class OSNetworkSystem implements 
> INetworkSystem {
>          throw new SocketException();
>      }
> 
> +    public int[] select(FileDescriptor[] readFDs, FileDescriptor[] 
> writeFDs, int countRead, int countWrite,
> +            long timeout) throws SocketException {
> +        int result = 0;
> +        if (0 == countRead + countWrite) {
> +            return (new int[0]);
> +        }
> +        int[] flags = new int[countRead + countWrite];
> +
> +        assert validateFDs(readFDs, writeFDs, countRead, countWrite) : 
> "Invalid file descriptor arrays"; //$NON-NLS-1$
> +
> +        // handle timeout in native
> +        result = selectImpl(readFDs, writeFDs, countRead, countWrite, 
> flags,
> +                timeout);
> +
> +        if (0 <= result) {
> +            return flags;
> +        }
> +        if (ERRORCODE_SOCKET_TIMEOUT == result ||
> +            ERRORCODE_SOCKET_INTERRUPTED == result) {
> +            return new int[0];
> +        }
> +        throw new SocketException();
> +    }
> +
> +
>      private native int selectImpl(FileDescriptor[] readfd,
>              FileDescriptor[] writefd, int cread, int cwirte, int[] flags,
>              long timeout);
> @@ -497,6 +522,22 @@ final class OSNetworkSystem implements 
> INetworkSystem {
>          return true;
>      }
> 
> +    private boolean validateFDs(FileDescriptor[] readFDs,
> +            FileDescriptor[] writeFDs, int countRead, int countWrite) {
> +        for (int i = 0; i < countRead; ++i) {
> +            // Also checks fd not null
> +            if (!readFDs[i].valid()) {
> +                return false;
> +            }
> +        }
> +        for (int i = 0; i < countWrite; ++i) {
> +            if (!writeFDs[i].valid()) {
> +                return false;
> +            }
> +        }
> +        return true;
> +    }
> +
>      /**
>       * Write bytes from a byte array to a socket.
>       *
> Index: 
> modules/nio/src/main/java/common/org/apache/harmony/nio/internal/SelectorImpl.java 
> 
> =====================================================================
> --- 
> modules/nio/src/main/java/common/org/apache/harmony/nio/internal/SelectorImpl.java 
> 
> +++ 
> modules/nio/src/main/java/common/org/apache/harmony/nio/internal/SelectorImpl.java 
> 
> @@ -469,13 +469,12 @@ final class SelectorImpl extends AbstractSelector {
>                      doCancel();
>                      int[] readyChannels = null;
>                      boolean isBlock = (SELECT_NOW != timeout);
> -                    prepareChannels();
>                      try {
>                          if (isBlock) {
>                              begin();
>                          }
>                          readyChannels = 
> Platform.getNetworkSystem().select(
> -                                readableFDs, writableFDs, timeout);
> +                                readableFDs, writableFDs, 
> readableKeysCount, writableKeysCount, timeout);
>                      } finally {
>                          if (isBlock) {
>                              end();
> 
> 

Hi Jesse,

I proposed a serial of patches on JIRA, that combines part of work from you, 
what are your options? Is it OK to commit them?

-- 
Best Regards,
Regis.

Mime
View raw message