hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sam Berlin" <sber...@gmail.com>
Subject Re: svn commit: r724147 - /httpcomponents/httpcore/trunk/module-nio/src/main/java/org/apache/http/impl/nio/reactor/IOSessionImpl.java
Date Sun, 07 Dec 2008 18:21:58 GMT
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,  <olegk@apache.org> 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: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org


Mime
View raw message