Why does this code produce a race condition?

Question

This is my first attempt at using std::future.

I have three different files that I want to parse at the same time. Three functions do that, resp. called parseSentences, parseTags and parseLinks. Each of them is launched in a separate thread using std::async by using a very simple lambda function: []() { parser->function(); }, where parser is a static variable and function is one of the three functions I have named previously.

int parser::start()
{
    int ret = SUCCESS;
    ASSERT( m_output != nullptr );
    static parser * thisParserInstance = this;

    // parsing files
    std::future<int> parseSentence = std::async(std::launch::async, []() { return thisParserInstance->parseSentences(); } );
    std::future<int> parseLinksResult = std::async(std::launch::async, []() { return thisParserInstance->parseLinks(); } );
    std::future<int> parseTagsResult = std::async(std::launch::async, []() { return thisParserInstance->parseTags(); } );

    // retrieving the results
    ret = parseSentence.get();
    const int linksResult = parseLinksResult.get();
    const int tagsResult = parseTagsResult.get();

    if (ret == SUCCESS)
        ret = linksResult == SUCCESS ? tagsResult : linksResult;

    return ret;
}

Now when I run my program in gdb, a segmentation fault occurs at the destruction of one of the std::future local variable. There are 2 threads running at that moment. Thread #1’s call stack is here. Thread #2’s call stack is here.

Note that the pointer to this in the first call stack is null, resulting in the segmentation fault.

If anyone has a clue, I would be thankful.

Without looking at the bug, this design still seems scary. What does `thisParserInstance` exist? It's name is incorrect: it's the first this to enter the function's instance, *forever*. Why not use `this` directly? — GManNickG, Aug 27 '12 at 22:15
Why so complicated? Just put `[this]` in the capture list... — Kerrek SB, Aug 27 '12 at 22:17
@GManNickG I should have explained that. This function is only called once during the program execution. I needed to pass the parser instance to the 3 new threads , and I couldn't see any cleaner way to do so. — qdii, Aug 27 '12 at 22:18
You should post a complete program which has this issue, so we don't have to guess about what you have done elsewhere. — Mankarse, Aug 27 '12 at 22:20
Since the parser instance is passed to all three threads, it immediately raises the question of what's being done with that instance. If you suspect a data race, the parser instance is the most obvious culprit. — Pete Becker, Aug 27 '12 at 22:23
@PeteBecker that’s true. Yet the fact that the first line of the first callstack shows a member function called on a `std::__future_base` which `this` pointer is null lets me think that the one of the future object is destroyed before its value is affected. — qdii, Aug 27 '12 at 22:26
A data race results in undefined behavior. Once that happens, there's very little point in reasoning about the state of the program or its data. Chances are that the library implementation is right and the problem is in the code you wrote. — Pete Becker, Aug 27 '12 at 22:31
From the stack traces this looks like http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54297 which I fixed two days ago. I have no idea why linking to libpthread would help, the [first stack trace](http://pastebin.com/yrBTDre6) shows a thread has already been created (by `clone()`) which wouldn't be possible if the program wasn't already linked to libpthread. — Jonathan Wakely, Aug 27 '12 at 23:25

Mike Seymour · Accepted Answer · 2012-08-28T12:26:57.343

3

One big problem is here:

static parser * thisParserInstance = this;

That's initialised the first time you call the function, and then left unchanged on future calls. So if you call the function on one object, destroy that object, and then call it on a second object, you'll actually be working on a dangling pointer to the defunct object. That will certainly give undefined behaviour.

There's no reason to use a static variable; the lambdas can capture this and act on the correct object. Or more simply, as suggested in the comments, use the variadic form of async to bind this to the member function:

std::async(std::launch::async, &parser::parseSentences, this);

edited Aug 28 '12 at 12:26

answered Aug 27 '12 at 22:17

Mike Seymour

249,747
28
448
644

+1, thanks. I will change that, but this can’t be the bug as the function is only called once, on purpose. – qdii Aug 27 '12 at 22:19
Try capturing the this-ptr anyway, even though you do not think it will help. I think you may be surprised. – Jive Dadson Aug 27 '12 at 22:29
4

There's no need to use a lambda anyway, `std::async(std::launch::async, &parser::parseSentences, this);` would work fine. – Jonathan Wakely Aug 27 '12 at 23:21

score 1 · Answer 2 · edited May 23 '17 at 12:20

1

Sorry guys.

here is the solution: std::future exception on gcc experimental implementation of C++0x

After linking with -lpthread, the error disappeared. Thanks for your other remarks, though, they were very helpful.

edited May 23 '17 at 12:20

Community

1
1

answered Aug 27 '12 at 22:36

qdii

12,505
10
59
116

I suspected that std::future was not introducing a memory barrier. Maybe that is what -lpthread accomplishes. – Jive Dadson Aug 27 '12 at 22:52
I don't think that's the problem, the code is already linked to libpthread as you can clearly see from the fact one of the stack traces shows it executing code from libpthread: `#18 0x00007ffff64eaec6 in start_thread () from /lib64/libpthread.so.0` – Jonathan Wakely Aug 27 '12 at 23:27
@qdii: Have a look at the requirements in the [libstdc++ manual](http://gcc.gnu.org/onlinedocs/libstdc++/manual/using_concurrency.html). – Mankarse Aug 28 '12 at 01:01
Uhm, is that the only change you made, adding -lpthread? Or did you capture the this-pointer also? – Jive Dadson Aug 28 '12 at 04:52

Why does this code produce a race condition?

2 Answers2