• (nodebb)

    Even before seeing the call site, I was bugged by how the function can be called without trying even ONCE.

  • my name is missing (unregistered)

    Also given the inaccuracies of clocks, it's entirely possible at 100ms that this executes or not randomly.

  • Robin (unregistered)

    While I don't deny this is a WTF, it's easy to see how this likely came about. It's because the function uses a hard coded 100ms as an implementation detail, and there's no reason to expect the caller to need to know about it. Perhaps that hardcoded time interval within the function has changed over time and/or the time interval in the calling code has - it's all too common for changes like that eventually cause code to change from perfectly OK to somewhat WTFy.

    There will absolutely be better ways of doing this, but I won't attempt to suggest any - partly because I don't know C++ at all, and partly because it depends on what the intended use cases of the function are.

    Certainly, if it turns out that neither the interval in the function or the one at the call site have ever changed, and this function is never called anywhere else, then this is utterly dumb. Otherwise I can't feel too strongly about it - it's not good, but I see worse more or less every day.

  • Dan Bugglin (google)

    I'm bugged by how this C++ code apparently allows for numerical units to be specified.

    Though now that I think about it I heard about a concept like that... you can tag a string variable as a "phone number" or "e-mail" type and then you can't assign incompatible data types to that variable.even if they are strings, as long as all your variables are tagged properly.

  • Anonymous') OR 1=1; DROP TABLE wtf; -- (unregistered)

    WTFs in this code:

    1. The sleep call should use std::min(100ms, timeToWait - (now() - start)) to ensure it doesn't wait longer than the deadline (and now() should be cached in a local so that it's not immediately evaluated twice in a row).

    2. Depending on which clock implementation the now() function uses, it's likely it'll break on clock adjustments. If the clock is non-monotonic and gets set backwards during the loop, either by the system administrator or by an NTP update, then this could be sleeping for a long time. Or worse, if now() uses local time instead of UTC, then that will also happen during DST changes or time zone changes. (Then there's also theoretically negative leap seconds, but those have never happened yet and are rather unlikely to happen in the future.)

    3. The retry logic should have exponential backoff with random jitter added—each iteration through the loop should double the sleep time and add jitter until the deadline is met. This is to avoid the thundering herd problem. If the server or network is down and then suddenly comes back up, you don't want all of the clients suddenly waking up at the exact same time and overloading the server; you want them to spread out the load.

    Of course, given that this code is only ever called with a 100ms deadline, (2) and (3) aren't problematic yet.

    @Dan Bugglin: This is a (not-so-)new feature in C++11 called user-defined literals. The std::chrono library defines the ms literal (as of C++14) to be an expression of type std::chrono::milliseconds (https://en.cppreference.com/w/cpp/chrono/operator%22%22ms).

  • (nodebb) in reply to R3D3

    The cure for that is to convert the while loop into a do-while loop, but the fundamental issue remains. This thing will still never retry with the given arguments.

    Mind you, this thing will never compile: [bstart[/b] is not defined (should be std::chrono::system_clock::time_point start; at the beginning of the function or this supposed to be a global?!), and system_clock is not a namespace so has to be defined on now(). Is there some using statement that can make this valid?

    The best I could do to reproduce this is (with my change):

    using namespace std::chrono;

bool receiveData(uint8_t** data, std::chrono::milliseconds timeToWait) {
    system_clock::time_point start;

    start = system_clock::now();
    do {
        if (/* successfully receive data */) {
            return true;
        }
        std::this_thread::sleep_for(100ms);
    } while ((system_clock::now() - start) < timeToWait);
    return false;
}
  • (nodebb) in reply to Anonymous') OR 1=1; DROP TABLE wtf; --

    You can eliminate #2 [pun intended] by limiting the comparison to, say, just the seconds portion of now() . Then no matter who resets what master clock, things can't take more than 60 seconds for the timeout to fire.
    If you've got a situation where you need to keep trying for longer timeout periods than one minute, there are other more pressing problems than a timeout.

  • gnasher729 (unregistered) in reply to Anonymous') OR 1=1; DROP TABLE wtf; --

    I'm doing mostly iOS development at the moment. There is a builtin class NSDate which returns the time in seconds since epoch in UTC, depending on the clock settings of the device. So it will jump when you change the time on your iPhone (but not the timezone). Caused problems when a notification was supposed to be shown for ten seconds, and user set the clock back by an hour.

    So I added a class NSTime which counts seconds since some unknown time, probably related to the time the app launched or the phone was rebooted. And that count changes by quite exactly one second every second, no matter what you do. So if you want to remove a notification after ten seconds, this allows you to remove it after ten seconds (or at the next possible opportunity if you lock your phone).

    That's what you should do, write a class like that once, and then the problem is solved.

  • (nodebb) in reply to cellocgw

    just the seconds portion of now() .

    That introduces a different bug, where calling this function 95ms before a minute boundary causes a 60 second wait instead of a 100ms one.

    It turns a potentially long but very irregular hang (maybe one in ten million chance of hitting an NTP update, admin action, or DST change) into a fairly short (only one minute) but very likely hang (one in 600 chance).

  • Just another Embedded Designer (unregistered)

    Classic cases of "I have a hammer everything is a nail" and I only know PRESENTATION layer decsriptions.

    1/ For a SHORT, delay and/or timeout WHY are you using complicated date/time methods? This is a relative timing not fixed to planetary motion positions.

    2/ This should be using the normal thing (that may not have API or OS accesible methods), which is UPTIME counters of milliseconds or microseconds, this is how lower level schedling of tasks and task run times use for simplicity. The use of these like adding a task to be schedule on specific day may well have that date stored, but does comprisons on UPTIME to do actual execution checks.

    3/ Too often I see these WRONG deadline calculations and recalculations in loops, just enter get current uptime and calculate the END time for your deadline time and use that for comparisons.

    On many embedded and other systems I even create subtask scheduling, deadlines, timeouts using simple UPTIME counters, these days it is easy to create milli-second or micro-second uptime counters that are 64 bit long, using unsigned maths for short delays and items that are NOT scheduled to run on calendar events once a month or even year, easy to implement.

    now() is a localised PRESENTATION layer nicety being used incorrectly here.

  • MZH (unregistered) in reply to Just another Embedded Designer

    All of your points are addressed by using std::chrono::steady_clock: no date/time calculations, just a monotonic count of ticks since some constant point in the past. The clock std::chrono::system_clock is for date/time calculations.

Leave a comment on “Sleep on It”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article