Given that Aleksey and I came up with essentially the same patch to help
alleviate this instability -- is there any support for putting it into
the code now (ahead of M4 testing)? I believe it is a relatively
localized and minor change even at this stage.
Regards,
Tim
Tim Ellison wrote:
> 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) {
>
