I'm fairly certain this change will have unintended consequences. One of the quirks about interestOps is that they are legally allowed to be set from any thread -- this is what allows a currently-blocked selector to wakeup seeing some new interestOps (when coupled with a selector.wakeup). The simple fix here would be to make the new local interestOps variable volatile, but that has subtle bugs. All mutations of interestOps require a read, a stack-local mutate and then a set, which means that it's possible for one thread to begin a read & mutate, then another to begin the same procedure. When context returns to the first thread, the resulting set loses the mutate from the second thread. It's a classic thread-safety problem due to lack of a critical section.
It's possible to do some sort of CaS non-blocking operation similar to how the internals of AtomicInteger implement getAndDecrement, getAndAdd, etc... but it's far easier to just re-add the critical section (ie, 'synchronized') around usage of interestOps. Sam On Sun, Dec 7, 2008 at 11:11 AM, <[EMAIL PROTECTED]> wrote: > Author: olegk > Date: Sun Dec 7 08:11:57 2008 > New Revision: 724147 > > URL: http://svn.apache.org/viewvc?rev=724147&view=rev > Log: > Maintain a local copy of interest ops > > Modified: > > httpcomponents/httpcore/trunk/module-nio/src/main/java/org/apache/http/impl/nio/reactor/IOSessionImpl.java > > Modified: > httpcomponents/httpcore/trunk/module-nio/src/main/java/org/apache/http/impl/nio/reactor/IOSessionImpl.java > URL: > http://svn.apache.org/viewvc/httpcomponents/httpcore/trunk/module-nio/src/main/java/org/apache/http/impl/nio/reactor/IOSessionImpl.java?rev=724147&r1=724146&r2=724147&view=diff > ============================================================================== > --- > httpcomponents/httpcore/trunk/module-nio/src/main/java/org/apache/http/impl/nio/reactor/IOSessionImpl.java > (original) > +++ > httpcomponents/httpcore/trunk/module-nio/src/main/java/org/apache/http/impl/nio/reactor/IOSessionImpl.java > Sun Dec 7 08:11:57 2008 > @@ -53,6 +53,7 @@ > private final SessionClosedCallback callback; > private final Map<String, Object> attributes; > > + private int interestOps; > private SessionBufferStatus bufferStatus; > private int socketTimeout; > > @@ -65,6 +66,7 @@ > this.channel = (ByteChannel) this.key.channel(); > this.callback = callback; > this.attributes = Collections.synchronizedMap(new HashMap<String, > Object>()); > + this.interestOps = key.interestOps(); > this.socketTimeout = 0; > this.status = ACTIVE; > } > @@ -92,13 +94,14 @@ > } > > public int getEventMask() { > - return this.key.interestOps(); > + return this.interestOps; > } > > public void setEventMask(int ops) { > if (this.status == CLOSED) { > return; > } > + this.interestOps = ops; > this.key.interestOps(ops); > this.key.selector().wakeup(); > } > @@ -107,10 +110,8 @@ > if (this.status == CLOSED) { > return; > } > - synchronized (this.key) { > - int ops = this.key.interestOps(); > - this.key.interestOps(ops | op); > - } > + this.interestOps = this.interestOps | op; > + this.key.interestOps(this.interestOps); > this.key.selector().wakeup(); > } > > @@ -118,10 +119,8 @@ > if (this.status == CLOSED) { > return; > } > - synchronized (this.key) { > - int ops = this.key.interestOps(); > - this.key.interestOps(ops & ~op); > - } > + this.interestOps = this.interestOps & ~op; > + this.key.interestOps(this.interestOps); > this.key.selector().wakeup(); > } > > > > --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
