bad assert() in Monitor.cpp

[Erik Frey <er...@last.fm>]

I just had a very long night because of line 69 in 
lib/cpp/src/concurrency/Monitor.cpp:

assert(Util::currentTime() >= (now + timeout));

There are a couple of problems with this and I thought I'd check here 
before submitting a jira:
* Release version of thrift library isn't being compiled with -DNDEBUG
* On certain kernels, waiting with a very long timeout (e.g. many 
minutes or hours) returns just -before- the expected time, hitting the 
assert and abruptly bringing down the whole process.  May be a timer 
resolution issue, not sure.

What do you guys suggest?  Whether -DNDEBUG is used or not, I'm not sure 
thrift release is an appropriate place to unit test pthreads.

Erik

RE: bad assert() in Monitor.cpp

[Mark Slee <ms...@facebook.com>]

I remember this specific issue coming up before (i.e. the pthread library doesn't strictly obey timeouts and may actually return early) -- though I can't remember exactly what we did to address it. It may have only come up internally at Facebook...

Aside from turning off -DNDEBUG I might recommend actually commenting out this assert statement and placing a comment next to it explaining that the underlying thread implementation doesn't actually properly support these semantics. If possible we should #ifdef the assert() for just the platforms where we know the timeout handling to be an issue -- generally a failure of this assert() could represent a major problem.

-----Original Message-----
From: Erik Frey [mailto:erikf@last.fm] 
Sent: Wednesday, November 26, 2008 9:59 AM
To: thrift-dev@incubator.apache.org
Subject: bad assert() in Monitor.cpp

I just had a very long night because of line 69 in
lib/cpp/src/concurrency/Monitor.cpp:

assert(Util::currentTime() >= (now + timeout));

There are a couple of problems with this and I thought I'd check here before submitting a jira:
* Release version of thrift library isn't being compiled with -DNDEBUG
* On certain kernels, waiting with a very long timeout (e.g. many minutes or hours) returns just -before- the expected time, hitting the assert and abruptly bringing down the whole process.  May be a timer resolution issue, not sure.

What do you guys suggest?  Whether -DNDEBUG is used or not, I'm not sure thrift release is an appropriate place to unit test pthreads.

Erik