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();
--
Best Regards,
Regis.
