kipr / libkovan

Standard library for the kovan robot controller
GNU General Public License v3.0
6 stars 5 forks source link

Multithreading not supported #2

Open milch opened 11 years ago

milch commented 11 years ago

Yesterday at ECER 2013 our robot crashed multiple times. When I looked at our code today I couldn't find out what was wrong, so I decided to look at the libkovan code... Turns out, our problems were the threads we were using! I only looked through some of the files in the code, however, when looking at something like the following:

void Kovan::enqueueCommand(const Command &command, bool autoFlush)
 {
         m_queue.push_back(command);

         // FIXME: This logic needs to be improved eventually
         if(autoFlush) autoUpdate();
}

it became apparent that the library is not ready for multi-threading at all! When enqueuing from multiple threads, this becomes a classic read/write race-condition. You should look at this link, which explicitly states that STL containers are not guaranteed to be thread-safe and that it can cause problems when enqueuing from multiple threads and reading from another one (which is exactly what happens here).

Another example would be the following pattern, which is present throughout the kovan library:

Analog *Analog::instance()
{
    static Analog s_analog;
    return &s_analog;
}

Depending on the compiler used, this is also not thread-safe. The C++03 standard stays silent on this matter, so it's not safe to assume that the code the compiler produces will be thread-safe. However, if you are using C++11 then this becomes irrelevant (though I have no reason to believe that you are using C++11 from the code I have seen, because none of the newer features seem to be present).

Until this issue is resolved (and it is very likely going to be a time consuming process to convert the whole codebase to be thread-safe and to sort out all the multithreading bugs) I would advise you to deprecate the threading functions in the libkovan documentation as well as issue a warning not to use any hardware-related functionality from a thread other than the main-thread. The reason to this is that the current documentation mentions nothing of this, which can be irritating for people and lead them to believe that libkovan natively supports multithreading (like we believed... unfortunately, no bugs ever occured before the tournament).

bmcdorman commented 11 years ago

I am intimately aware that libkovan isn't thread safe. Apologies for not making this clearer in the documentation. This is on the road map, but it is not high priority, as 95% of botball teams will not use threading. Thread safety was omitted simply because libkovan was written under extreme time constraints, and getting stuff working for the majority was the only priority. As a workaround, use a mutex.

milch commented 11 years ago

First of all, thanks for the quick answers.

I don't think that multi-threading is as little used as you think it is. If you look at the botball community forums on programming you can see that two of the most recent topics are on threading, and I know that at least some of the other teams we talked to during ECER were using multiple threads too (I don't remember how many teams there were exactly, though). At least one of the topics seems to have been created by a rookie programmer, so I don't think that only more advanced programmers are facing this problem ... A rookie programmer might not even be able to tell that a bug is caused by his use of multiple threads and in turn blame hardware, bad luck, etc.