You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Mark Hindess <ma...@googlemail.com> on 2007/12/07 10:12:53 UTC
Re: [classlib][nio] jetty corrupts big static files
On 6 December 2007 at 16:31, Tim Ellison <t....@gmail.com> wrote:
> Aleksey Shipilev wrote:
> > So far I've managed to reproduce this issue on SLES10 and
> > epoll()-based Selector. There are really weird results difference
> > between RHEL and SLES on the same build - I'm looking into the
> > differences on the native side calls now. As far as you might be more
> > familiar with legacy selector code, you might try to reproduce and
> > catch the problem there, then we can sync our visions on what's going
>
> Aleksey,
>
> 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.
> Given this is not a regression from M3, I suggest we downgrade it from
> critical to major, and put off any significant reworking until after
> the milestone.
>
> WDYT?
Seems reasonable.
I've started looking for obvious errors. I used the IBM VME with:
java -Xcheck:jni:all,pedantic,nonfatal -jar start.jar
to look for JNI issues and noticed several problems with the code for
throwJavaNetSocketException in nethelp.c:
1) errorMessageString is created but not used in the common code path
2) in the HYPORT_ERROR_SOCKET_WOULDBLOCK code path, several JNI calls
are not checked for exceptions - two NewObject, GetMethodID,
CallObjectMethod
3) in the HYPORT_ERROR_SOCKET_WOULDBLOCK code path Throw is called but
there is no return so the code simply falls through to the default
case. It is tempting to fix this by adding a return but this code
is effectively not being used so perhaps we should remove it?
The code for 2 and 3 came from HARMONY-815 please can the originator
take a look.
A more minor issue in
Java_org_apache_harmony_luni_platform_OSNetworkSystem_selectImpl is that
after the poll we are setting outFlags to SOCKET_OP_NONE which is zero
and since this array is initialised to zero anyway then this is a little
pointless. Better to remove these. We can also then keep a count of
any changes made and do use ReleaseIntArrayElements with JNI_ABORT if
there are no changes - which seems to be a common case in the jetty
test.
Regards,
Mark.
P.S. check:jni is really useful we should test more of our native code
with it.
Re: [classlib][nio] jetty corrupts big static files
Posted by Tim Ellison <t....@gmail.com>.
Alexey Petrenko wrote:
> +1 for commit.
Thanks Alexey -- committed at r602572.
Regards,
Tim
> 2007/12/9, Tim Ellison <t....@gmail.com>:
>> 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 <ma...@googlemail.com> 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) {
>>>
>
Re: [classlib][nio] jetty corrupts big static files
Posted by Alexey Petrenko <al...@gmail.com>.
+1 for commit.
2007/12/9, Tim Ellison <t....@gmail.com>:
> 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 <ma...@googlemail.com> 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) {
> >
>
Re: [classlib][nio] jetty corrupts big static files
Posted by Tim Ellison <t....@gmail.com>.
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 <ma...@googlemail.com> 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) {
>
Re: [classlib][nio] jetty corrupts big static files
Posted by Tim Ellison <t....@gmail.com>.
Aleksey Shipilev wrote:
> Hi, Mark!
>
> On Dec 7, 2007 12:12 PM, Mark Hindess <ma...@googlemail.com> 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) {
Re: [classlib][nio] jetty corrupts big static files
Posted by Aleksey Shipilev <al...@gmail.com>.
Hi, Mark!
On Dec 7, 2007 12:12 PM, Mark Hindess <ma...@googlemail.com> 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?
Thanks,
Aleksey.