Re: svn commit: r724147

[Oleg Kalnichevski <ol...@apache.org>]

Sam Berlin wrote:
> 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.
> 

Good catch. My bad. I re-introduced the critical section around 
interestOps and made interestOps volatile. Please review [1]

The reason I have done this change is to make it possible for the I/O 
session to defer mutation of SelectionKey's interestOps to the I/O 
dispatch in order to avoid the dead-lock condition described in 
HTTPCORE-155 [2] when running on buggy JREs such as IBM's

Oleg

[1] http://svn.apache.org/viewvc?rev=724177&view=rev
[2] https://issues.apache.org/jira/browse/HTTPCORE-155




---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org

Re: svn commit: r724147

[Oleg Kalnichevski <ol...@apache.org>]

Sam Berlin wrote:
>> Good catch. My bad. I re-introduced the critical section around interestOps
>> and made interestOps volatile. Please review [1]
>>
>> The reason I have done this change is to make it possible for the I/O
>> session to defer mutation of SelectionKey's interestOps to the I/O dispatch
>> in order to avoid the dead-lock condition described in HTTPCORE-155 [2] when
>> running on buggy JREs such as IBM's
> 
> I'm not positive the changes (or those in r724457) will do anything to
> resolve the issue on the IBM JREs.  The issue is caused by very poor
> javadoc recommendations on SelectionKey -- as Marc pointed out, "...
> In a naive implementation, reading or writing the interest set may
> block indefinitely if a selection operation is already in progress".
> In a nutshell, this means that if the selector is blocked on a
> select(), it is impossible to tell it ask a SelectionKey to change its
> interest.  This makes no sense in a high-performing NIO application,
> as interest changes can happen from any thread and selector.wakeup()
> tells the selecting-thread to take a new look at the interest.
> 
> I think the code as it is now (with r724457) will actually hinder
> speed in the already-working case and be ineffective in the
> not-working case.  (That said, they won't break the already-working
> case.)  This is because there's no an additional mutex required for
> changing interestOps -- the local one and the one implicitly gotten
> when calling setInterestOps on a SelectionKey.
> 
> There's a few approaches that can help the IBM JRE case.  One, which
> might not work all the time, but should work fairly reliably, would be
> to call selector.wakeup() prior to grabbing the SelectionKey's mutex.
> You'll still need to call selector.wakeup() after setting the ops,
> though, because of the problem with the approach.  The selector may
> wakeup & reselect before the ops are actually set.  In Sun's case,
> this would just be a temporary problem -- the additional wakeup()
> afterwards will fix things.  In IBM's case, it would lead right back
> to the deadlock.
> 
> The other approach would be to somehow pass the requested ops into the
> selecting thread & have the thread calling select() set the interest
> ops.  This can be done by storing the ops locally, telling the
> selector to wakeup, and having the selecting thread iterate through
> all of it's channels prior to the next select and resetting the
> interestOps before selecting.  That should fix the problem on all
> JREs, but may be architecturally difficult.
> 


Hi Sam,

These changes were not meant to resolve the issue, but rather make it 
easier to override the way interestOps were manipulated by IOSession 
implementations. I am sorry if I failed to make that clear.

Anyways, reverted the changes for now. Let's revisit the issue in the 
course of 4.1 development.

> (Sorry for the delay in responding... been working furiously to
> release an alpha version of LimeWire 5, now finally released.)
> 
> Sam

Many thanks for reviewing the changes and congrats for the release.

Cheers

Oleg


> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
> For additional commands, e-mail: dev-help@hc.apache.org
> 


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org

Re: svn commit: r724147

[Sam Berlin <sb...@gmail.com>]

> Good catch. My bad. I re-introduced the critical section around interestOps
> and made interestOps volatile. Please review [1]
>
> The reason I have done this change is to make it possible for the I/O
> session to defer mutation of SelectionKey's interestOps to the I/O dispatch
> in order to avoid the dead-lock condition described in HTTPCORE-155 [2] when
> running on buggy JREs such as IBM's

I'm not positive the changes (or those in r724457) will do anything to
resolve the issue on the IBM JREs.  The issue is caused by very poor
javadoc recommendations on SelectionKey -- as Marc pointed out, "...
In a naive implementation, reading or writing the interest set may
block indefinitely if a selection operation is already in progress".
In a nutshell, this means that if the selector is blocked on a
select(), it is impossible to tell it ask a SelectionKey to change its
interest.  This makes no sense in a high-performing NIO application,
as interest changes can happen from any thread and selector.wakeup()
tells the selecting-thread to take a new look at the interest.

I think the code as it is now (with r724457) will actually hinder
speed in the already-working case and be ineffective in the
not-working case.  (That said, they won't break the already-working
case.)  This is because there's no an additional mutex required for
changing interestOps -- the local one and the one implicitly gotten
when calling setInterestOps on a SelectionKey.

There's a few approaches that can help the IBM JRE case.  One, which
might not work all the time, but should work fairly reliably, would be
to call selector.wakeup() prior to grabbing the SelectionKey's mutex.
You'll still need to call selector.wakeup() after setting the ops,
though, because of the problem with the approach.  The selector may
wakeup & reselect before the ops are actually set.  In Sun's case,
this would just be a temporary problem -- the additional wakeup()
afterwards will fix things.  In IBM's case, it would lead right back
to the deadlock.

The other approach would be to somehow pass the requested ops into the
selecting thread & have the thread calling select() set the interest
ops.  This can be done by storing the ops locally, telling the
selector to wakeup, and having the selecting thread iterate through
all of it's channels prior to the next select and resetting the
interestOps before selecting.  That should fix the problem on all
JREs, but may be architecturally difficult.

(Sorry for the delay in responding... been working furiously to
release an alpha version of LimeWire 5, now finally released.)

Sam

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org