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();
