The patch looks good to me..
There is another small basic problem .... we set up most time intervals in
microseconds before invoking corresponding apr functions. Most user/test
time intervals are in milliseconds. So, it need a (x * 1000) kind of
operation. On linux, apr invokes the posix api's which take microsecond
arguments, so all the tests run fine on Linux. On Windows, most timeouts are
in milliseconds. So the apr code does a (x / 1000 ) kind of operation to
convert back the microseconds into millis. This is really poor rounding and
drops nanoseconds etc. in the test interval, which ThreadTest etc. try to
use. I would suggest that we make a trivial change to:
build/patches/win/apr/locks/win32/thread_cond.c
===================================
APR_DECLARE(apr_status_t) apr_thread_cond_timedwait(apr_thread_cond_t *cond,
apr_thread_mutex_t * mutex, apr_interval_time_t timeout )
{
DWORD timeout_ms = (DWORD) apr_time_as_msec(timeout);
+ if (timeout % 1000 ) timeout_ms ++;
return _thread_cond_timedwait(cond, mutex, timeout_ms );
}
in addition to Elena's changes.
Thanks,
Rana
On 11/29/06, Elena Semukhina <[EMAIL PROTECTED]> wrote:
After a few days of iterative runs of kernel tests on windows/linux I
gathered some statistics on failures.
I've prepared a patch for ThreadTest and ObjectTest and attached it to
HARMONY-2204. It fixes testJoinLongInt() and some similar tests,
eliminates
testYield() failures on linux and does code clean up to get rid of some
useless waiting. It also adds a proper waiting for a thread's actual start
to prevent sporadic failures.
Could someone look at the patch and evaluate it?
Thanks,
Elena
On 11/28/06, Elena Semukhina <[EMAIL PROTECTED]> wrote:
>
>
>
> On 11/28/06, Alexei Fedotov <[EMAIL PROTECTED]> wrote:
> >
> > Elena,
> > As far as I understand from your letter the implementation is still to
> > be fixed. Could you please file a JIRA about it?
>
>
> I'm not sure that the implementation is to be fixed as it does not
> contradict the spec for now :)
> Possibly we should fix the test so that it waits e.g. 10% less. Anyway,
> I've filed http://issues.apache.org/jira/browse/HARMONY-2204 some time
ago
> for this problem. The patch I suggested is invalid because the test
> continues failing with new timing numbers (I gave 1 ms of freedom but
this
> was not enough).
>
> Geir,
> > As for the best of my knowledge there are no reliability runs for
> > kernel tests. This case proves that trying to run these tests in a
> > loop, especially in one JVM could be useful for detecting new
> > implementation flaws.
>
>
> It is me who runs these tests iteratively for the last week. The runs
> proved that ThreadTest fails sporadically on waiting in join() and
sleep()
> methods on Windows and in yield() on linux. I'll file aJIRA issue for
the
> latter.
>
> Thanks,
> Elena
>
> --
> > Thank you,
> > Alexei
> >
> >
> > On 11/27/06, Elena Semukhina < [EMAIL PROTECTED]> wrote:
> > > On 11/28/06, Elena Semukhina <[EMAIL PROTECTED]> wrote:
> > > >
> > > > The problem with ThreadTest has not been fixed yet. I'm running
this
> > test
> > > > iteratively now to see the sporadic failures. Indeed,
> > testJoinlongint()
> > > > fails rather often. The spec for join() reads that it should wait
at
> > most
> > > > millis milliseconds for this thread to die. But the thread which
is
> > > > joining in the test does not stop
> > > >
> > >
> > > The obvious typo: I meant "the thread which is joined".
> > >
> > >
> > > running. Can we allow join() to exit earlier in this case?
> > > >
> > >
> > > I modified the test so that to let join() wait for10 ms less (which
> > > is 0.5%of waiting time) and ran it again iteratively. It failed once
> > > with 1972 ms
> > > of waiting!
> > >
> > > I looked through RI's bug database for relevant issues and found the
> > > following:
> > >
> > > http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4132653
> > > http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=5068368
> > >
> > > The last comment of the latter says that the spec will be fixed in
> > Java SE
> > > 6.0!
> > >
> > > BTW, the test always passes on linux for me.
> > >
> > > Thanks,
> > > Elena
> > >
> > >
> > > > On 11/28/06, Rana Dasgupta <[EMAIL PROTECTED] > wrote:
> > > > >
> > > > > I may have missed it, it's very possible, could someone point
me
> > to the
> > > > > fix
> > > > > or the thread ? My comments are based on what's in Harmony drlvm
> > trunk
> > > > > as of
> > > > > a few hours ago.
> > > > >
> > > > > Thanks,
> > > > > Rana
> > > > >
> > > > >
> > > > > On 11/27/06, Geir Magnusson Jr. < [EMAIL PROTECTED]> wrote:
> > > > > >
> > > > > > I thought this exact problem was identified and already
fixed...
> > > > > >
> > > > > > geir
> > > > > >
> > > > > > Rana Dasgupta wrote:
> > > > > > > Hi,
> > > > > > > On Windows XP, java.lang.ThreadTest ( testJoinlongint
> > component )
> > > > > > keeps
> > > > > > > failing for me sporadically. Debugging this, I saw that the
> > problem
> > > > > was
> > > > > > > with
> > > > > > > the expiry interval on thread.join(millis, nanos ). Tests
> > based on
> > > > > timed
> > > > > > > waits are somewhat unpredictable on most platforms.
> > > > > > > In condvar_wait_impl(), we seem to set up the timeout
> > interval in
> > > > > > > microseconds before calling apr_thread_cond_timeout(). On
> > Windows,
> > > > > > > apr_thread_cond_timeout() implements using
> > WaitForSingleObject(
> > > > > event,
> > > > > > > timeout )....but the Windows timeout is in milliseconds, as
> > far as I
> > > > >
> > > > > > know.
> > > > > > > Is this not an error, or am I missing something? I did not
> > want to
> > > > > > change
> > > > > > > anything since condvar_wait_impl() is on the code path of
> > several
> > > > > timed
> > > > > > > waits.
> > > > > > > Also, on a less important note, the tests testJoinLong()
and
> > > > > > > testJoinLongint() seem to test to see that the thread.join
> > (milli,
> > > > > nano)
> > > > > > > timeout is "at least" equal to the specified interval. My
> > > > > understanding
> > > > > > is
> > > > > > > that this should be "at most" the specified interval. Any
> > ideas?
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Rana
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > > Elena
> > >
> > >
> > >
> > >
> > > --
> > > Thanks,
> > > Elena
> > >
> > >
> >
> >
> > --
> > Thank you,
> > Alexei
> >
>
>
>
> --
> Thanks,
> Elena
--
Thanks,
Elena
