Aleksey Shipilev wrote:
> Hi, Mark!
>
> On Dec 7, 2007 12:12 PM, Mark Hindess <[EMAIL PROTECTED]> wrote:
>>> It looks like the code in NIO selector could do with a good review,
>>> and at minimum a few comments if not a partial redesign to tidy up the
>>> data structures and which locks are required for which, etc.
>> I agree. Though it is tempting to revert the HARMONY-4869 optimizations
>> (see JIRA comment for the two svn diff commands to do this) since that
>> does seem marginally more stable to me.
> I wonder if fixing the current legacy selector will be easier - that's
> because HARMONY-4869 already has some tidying in data structures. I
> suspect there are synchronization problems while modifying the key
> already registered on the selector. Can you try wrap the modKey() [1]
> with synchronization as follows:
>
> void modKey(SelectionKey sk) {
> + synchronized (this) {
> + synchronized (keysSet) {
> // TODO: update indexes rather than recreate the key
> delKey(sk);
> addKey(sk);
> + }
> + }
> }
>
> ...and see the difference?
I came to the same conclusion and have been running with the following
patch for a while... I'll admit that I followed other places in grabbing
the selectedkeys lock too, and it may not be necessary:
Index: main/java/common/org/apache/harmony/nio/internal/SelectorImpl.java
===================================================================
--- main/java/common/org/apache/harmony/nio/internal/SelectorImpl.java
(revision 602067)
+++ main/java/common/org/apache/harmony/nio/internal/SelectorImpl.java
(working copy)
@@ -359,8 +359,14 @@
*/
void modKey(SelectionKey sk) {
// TODO: update indexes rather than recreate the key
- delKey(sk);
- addKey(sk);
+ synchronized (this) {
+ synchronized (keysSet) {
+ synchronized (selectedKeys) {
+ delKey(sk);
+ addKey(sk);
+ }
+ }
+ }
}
/**
@@ -578,6 +584,9 @@
return unaddableSelectedKeys;
}
+ /*
+ * Assumes calling thread holds locks on 'this', 'keysSet', and
'selectedKeys'.
+ */
private void doCancel() {
Set<SelectionKey> cancelledKeys = cancelledKeys();
synchronized (cancelledKeys) {
