Renaming CP.25 raii_thread, and fixing CP.26 detached_thread

[hsutter]

CP.25 raii_thread

The PR for this is now ready to merge into GSL here: microsoft/GSL#504
IMO the design looks fine. But do we really want to use the name "raii_thread"? Yes, that's technically what it is (for those who like using that acronym), but why not "joining_thread" or "scoped_thread"? ... or, best of all, "thread"!

Proposed resolution: Rename "raii_thread" to just gsl::thread. This type is what std::thread ought to have been, and it's identical to std::thread except that gsl::thread has a properly-joining destructor and does not have detach. And this is what namespaces are for. :)

CP.26 detached_thread

The GSL PR has split out "raii_thread" because I suggested "detached_thread" was problematic and less recommendable. So we should also do something about CP.26 which recommends "detached_thread" -- I think detaching is an antipattern we should not encourage, as Hans and others have pointed out many times (e.g., orderly shutdown is at best problematic and usually impossible).

Proposed resolution:

• Drop "gsl::detached_thread" and the current CP.26.
• Replace CP.26 with the inverse Guideline that says "never detach threads" (that’s an unsafe antipattern). Include an example showing how you can never safely join or interact with it during static destruction. Enforcement: Flag any call to std::thread::detach. What to do instead: Just stick a normal gsl::thread in a global object or container and then ignore it if you want… but it means better more-orderly shutdown by default plus the ability to actually join with it if you do want to refer to it later.

[cubbimew]

this would also close the old To-do issue that says "is there a good reason to detach threads?"

[AndrewPardoe]

Per our editor's discussion, people don't consistently write namespace names. The same name in different namespaces might lead to confusion. When we talk about it, we're often not precise about which we are talking about. If it's something different, it should have a different name. Possibly joining_thread?

@jwakely, can you comment on the detached thread issue? Does this work in the light of a long POSIX tradition?

[jwakely]

I'm sure "never detach threads" will upset someone, but I don't expect strong enough objections that it would prevent that guideline.

[BjarneStroustrup]

TC++PL4 says "do not detach a thread unless you absolutely have to".
PPP doesn't bother to mention detach, so it doesn't warn".
The only reason we don't have "never detach a thread" is caution.I think the time has come.

[BjarneStroustrup]

(re)naming of raii_thread:
thread - I think recycling "thread" in gsl would cause confusion and accusations that we were trying to change the standard. So, no.
raii_thread - I like it :-)
joining_thread - maybe, but it doesn't grab me
scoped_thread - I'm against all "scoped_*" names for names in scopes, but some people like them :-(

[jwakely]

If recycling a name from std is a concern, be aware of http://wg21.link/p0206

[AraHaan]

The only reason to detatch threads is probaly to execute coroutines in a separate thread. The way python does it is that it uses an event loop and itterators as ways to scedule coroutines as tasks and each task would have a new temporary thread it is ran in. And to be honest I think a major break for couroutines in C++ is to have someone who knows python and C++ to at least copy some things from python's asyncio. At least asyncio on python can give hints.

However even with coroutines, there is no garrentee that functions can run without possible race conditions. However most race contitions come from things like:

// Example loop till 255 is reached.
for (int i = 0; i < 255; ++i) {
    // do stuff.
    // they accidentally do this. Do not do this as it can make an race condition.
    for (int i = 0; i > 0; ++i) {
        // other things.
    }
}

Sadly the compiler does allow this too.

[galik]

What about gsl::local_thread? Being local to the scope or object it is defined in?

[jwakely]

It's not local if you return it from a function, or have it as a data member of a class, or a global, or a member of a global container.

[AraHaan]

true, anyway the following files can help implement coroutines (I think) into C++ v. next:

• https://github.com/python/cpython/blob/master/Modules/_asynciomodule.c
• https://github.com/python/cpython/tree/master/Lib/asyncio (folder with tons of files to look at)

But yeah this can at most give hints on how to do it (However you guys might not want an event loop for exectuting coroutines, but you never know).

sometimes you just love having to source dive into things. But their C code is so much cancer code. (makes me really want to make my own rewrite to latest python master branch to be total C++ to be a lot easier on my eyes)

[jwakely]

I don't see how coroutines are on-topic here.

[hsutter]

@jwakely wrote:

If recycling a name from std is a concern, be aware of http://wg21.link/p0206

Yes, but I think that P0206 is evidence in favor of GSL using the name joining_thread. In Jacksonville, P0206 was encouraged by a consensus of LEWG modulo finalizing a name (the group was evenly split between joining_thread and simple_thread and left it to the author to pick a name; wiki notes here, requires password), and then in Oulu was killed by controversy in a larger group but that was unrelated to the name (wiki notes here, requires password).

So the name joining_thread is both (a) popular with the people who want the feature, and (b) not going to be used by the standard or a TS anytime soon. That makes it just about perfect.

BTW, I don't think we want to set a precedent that an RAII type should be named "raii_"... that would be a huge number of types. :)

[villevoutilainen]

It gives me pause to push the notion of the proposed std::joining_thread being what std::thread should've always been, and I am concerned about the confusion of different libraries giving different semantics to one name, namespaced or not. Chalk this up as a +1 for gsl::joining_thread.

[villevoutilainen]

Note that scoped_thread has some downsides; the proposed joining_thread can move across scopes, it's not bound to a particular scope.

[BjarneStroustrup]

Good votes and reasons for joining_thread; I'll try that.

[villevoutilainen]

As far as detach goes, Howard has showed an interesting use case for it. It looks like this:

thread{date::get_tzdb}.detach();

There, get_tzdb has a magic-static singleton in it. Initializing that magic-static is a heavyweight operation. Wrapping that operation into a detached thread makes it run in a non-blocking fashion so that it's likely ready when it's needed.

[villevoutilainen]

And there's more such valid uses for detach: http://stackoverflow.com/a/43617125/576911

[hsutter]

@villevoutilainen: These seem to be great examples against .detach :)

Re Howard's example:

thread{date::get_tzdb}.detach();

There, get_tzdb has a magic-static singleton in it. Initializing that magic-static is a heavyweight operation. Wrapping that operation into a detached thread makes it run in a non-blocking fashion so that it's likely ready when it's needed.

Or that it's likely to crash a short-lived program that never asks for the value and returns from main and enters static destruction before the initialization is complete. IMO this is a great example of why .detach() is dangerous by construction -- the initialization of the magic static might still be going on by the time main ends in a short program (e.g., might even crash its own empty unit test). The only objection I've heard to detached threads is not to "detaching" work from the initiation site which is fine and desirable -- rather, the objection is to detaching the work from all future access by the program, so there's no way of finding out whether they're done. In the above code, the only way to determine whether it's done -- and whether it's safe for main to end -- would be to try to invoke get_tzdb again redundantly just to touch the magic static again to get the block-if-it's-not-yet-initialized side effect.

What I suggested earlier in the discussion is to have something like a global vector<gsl::joining_thread> background_threads; (protected by a mutex if necessary for concurrent access) to hold the "background" (aka "detached") threads. Then we can rewrite the above to ::background_threads.emplace_back(date::get_tzdb);, which automatically ensures that the program joins with the async init during static destruction, by construction, whether or not the program is short-lived and whether or not get_tzdb is ever called again.

@BjarneStroustrup: This could be a good example to capture in the writeup.

And there's more such valid uses for detach: http://stackoverflow.com/a/43617125/576911

Is that the link you intended? I only see one example showing "detach" on that page, and it is immediately followed by this reply:

Note, however, that detaching a thread is a design decision, not a coding convenience, and is only rarely appropriate. In this case it's not needed. Once the main thread wakes up it can join the three threads. – Pete Becker Apr 25 at 18:01

[BjarneStroustrup]

I gave my opinion in TC++PL4 and I'm unlikely to depart far from that. My best example of detach is a heartbeat. However, the basic counterargument to all suggestions for detach is that you can transfer ownership into a scope that's external to the scope we want to detach from - that's a variant of the ownership argument that permeate much of the guidelines. Unless you have a handle to a task somewhere, you can't ask if it has completed, is making progress, etc.
The usual argument for not joining by default is that a bug that causes a crash is better than the same bug hanging forever in an unanticipated implicit join. That argument must be addressed.

[HowardHinnant]

I should explain.

In the thread{date::get_tzdb}.detach(); example, the assumption is that the program is long running unless something very bad happens. In the case that you are shutting down main very quickly, something bad has happened, and you are crashing. It doesn't really matter if you are crashing because of whatever bad happened in main, or because get_tzdb() touched something already destructed. You're crashing.

If you need a early terminate environment more graceful than that, then by all means use it. My arguments for detach() aren't that they are always the best. It is only that they are sometimes useful and preferable. I personally use join() far more than I use detach(). My only point is that my use of detach() is nonzero and useful.

join() is a convenience function (one I find very useful). It can be reproduced by a skillful combination of detach(), mutex and condition_variable. If join() doesn't meet your needs, it is highly likely that a careful combination of mutex, condition_variable and detach can create a customized join that does meet your needs.

My only point is that detach is not evil. It is a part of a lower-level API that exists and is useful to build other higher-level API's with useful semantics different than join.

[HowardHinnant]

By all means put a "black diamond" next to detach. But when you do, realize that you should also put "black diamonds" on mutex and condition_variable. And by all means, put "double black diamonds" on atomics. And for those using memory_order or launder, put quintuple black diamonds!

There is no question detach is a sharp tool. But it by no means even comes close to the sharpest tools in our tool box.

[gdr-at-ms]

For the Core Guidelines, I believe a focus is what to recommend to the majority of programmers, most of whom aren't experts in fine details of lower-level facilities provided by the language. It is expected that an expert would look at a specific situation and say "nah, I think this situation can run counter to the general guidelines and here is why." In other words, things not recommended by the Guidelines aren't necessary so because they are judged "not useful". Look at reinterpret_cast, for example. It is not recommended not because it is not useful.

[villevoutilainen]

If "Or that it's likely to crash a short-lived program that never asks for the value and returns from main and enters static destruction before the initialization is complete." is true, we have a serious defect in the standard. [basic.start.term]/1 says that such destruction is done only for objects that have begun their lifetime, aka have completed their initialization. We don't seem to say that if the initialization is ongoing, the destruction should be pending that initialization. But I digress; I think it's fine to recommend against using detach, because the whole point of the guidelines is indeed to put the sharpest knives further away from innocent users. I believe my and Howard's concern is what it means to "flag" such uses, and how users can control whether a tool will tell them their code is wrong or not.

[BjarneStroustrup]

I do a bit of thinking about consequences of rules. There are often unintended consequences, but my main question is who does it affect and how. If we help a million programmers avoid a bug by forcing a few hundred professionals to write two lines instead of one, I usually deem it a clear win. I don't think detach is quite that clear-cut, but that's one way to look at it.

A rule in the standard is in some sense absolute and universal: If the standard bans something, nobody can do it (except, of course, that we are rather clever bypassing the intent of the wording). A rule in the guideline is just a guideline you can - usually at your peril - bypass it without leaving ISO C++.

[jwakely]

Nobody's suggesting removing detach() from the standard, just giving guidelines for most programmers. There are also guidelines that make it very difficult to implement std::vector, because it uses raw (non-owning) pointers internally, and implementing it in terms of something like span would just be awkward. That doesn't mean the guideline is wrong, or std::vector should be deprecated, it just means that there are times when intentionally operating outside the guidelines is necessary, but it's "experts only" stuff.

[hsutter]

I agree with all of the above, thank you -- and @HowardHinnant my apologies that I neglected to mention your further comments on that SO solution right after Pete's, I only saw Pete's reply at first because I had highlighted "detach" on the page and was focused on the four hits.

[HowardHinnant]

Could we at least not tell people falsehoods?

Include an example showing how you can never safely join or interact with it during static destruction

This is just patently untrue.

[HowardHinnant]

Suggested language might go along the lines of:

If you detach a thread, you become responsible for interacting with that thread. This is typically done with mutex-protected state or atomics, and signaling using condition_variables. Recommended practice is to use join() instead.

[BjarneStroustrup]

Thanks, howard

[hsutter]

Thanks Howard. I can live with that weaker wording for the Guidelines, except I would prefer not to even mention those techniques because I don't think they're supported by the standard.

I don't think that what I wrote at top is a falsehood, and I've heard many stronger statements made over multiple years of arguments about this in SG1. You should expect prominent SG1 participants (not me) to continue to argue that it is inherently unsafe to safely interact with a detached thread during static destruction in any way -- joining it, signaling a mutex to communicate with it, etc. AFAIK those techniques are not guaranteed to work in ISO C++. For example, even joining with a detached thread during static destruction can be problematic is if the thread has a thread_local object with a nontrivial destructor -- the place where the C++11 concurrency model was particularly inventive. Ville pointed out one of the wording reasons why -- many (possibly not all?) the things you might do with/in/to a thread during static destruction, including the things you suggested, are undefined behavior, at least unless something has changed since I last sat in an SG1 discussion about it.

I agree with your point that the Guidelines don't need to include extended commentary on these things, but I don't agree that they're not real problems. AFAIK, the techniques you mention are not guaranteed to work in C++11/14/17 in the case I mentioned, "during static destruction." -- I would be happy to be corrected! But if it's still true that they don't work, I don't think we should encourage them, and I would rather not even mention them.

[BjarneStroustrup]

In this discussion of detach(), I think we forgot the primary reason for the existence of detached_thread: to allow static analysis of ownership and lifetime patterns. One problem with std::thread is that we cannot know whether it outlives its scope, so we don't know if we can pass a pointer to it or receive a pointer from it. For that reason, I sort of still like detached_thread. Comments?

[HowardHinnant]

except I would prefer not to even mention those techniques because I don't think they're supported by the standard.

Here is portable, standard-conforming C++11 that detaches 10 threads, waits with an "or" for those to finish under 1s that can, then cancels the remaining threads, and then joins with the canceled threads before exiting main:

https://wandbox.org/permlink/5RjOy09nY89zqTwv

You are either arguing that this code doesn't exist, or that it is non-conforming. Can you be more specific?

Admittedly, I'm not playing around with trying to join with detached threads after main returns (during static destruction time). I have no motivation to bring that aspect into this discussion. I'm talking about safely, portably and effectively controlling detached threads before main exits.

I'm sure I could make some example code to play with detached threads during the atexit run, but I don't really see the point. There's no reason to discuss it for this guideline.

[HowardHinnant]

For example, it would not be too difficult to take my example and wrap it up in a thread_pool class, make a file-scope instance of thread_pool, and have ~thread_pool() do the cancel & join dance. This would contradict the "does not work during static destruction" argument.

[BjarneStroustrup]

My #1 comment on detach() is that instead vest ownership of the (otherwise detached) thread in a scope that lives for long enough. "Potentially forever" can be done with a global or a bunch of shared_ptrs.

Agreed? I would like to keep most programmers out of subtle discussions and standards concerns like the one above.

[hsutter]

@BjarneStroustrup: Agreed. The high-order bit for this thread is what the Guidelines should encourage -- I think we're all in violent agreement that the Guidelines do not need to get into debates about how sharp the knife is? (Though I think SG1 should and does.)

@HowardHinnant asked:

https://wandbox.org/permlink/5RjOy09nY89zqTwv
You are either arguing that this code doesn't exist, or that it is non-conforming. Can you be more specific?

I took a quick look and there seem to be race conditions on the "done" flags, so it's undefined behavior before we get to detaching. For detaching, as you mentioned this example doesn't illustrate the case I highlighted of 'during static destruction.'

For example, it would not be too difficult to take my example and wrap it up in a thread_pool class, make a file-scope instance of thread_pool, and have ~thread_pool() do the cancel & join dance. This would contradict the "does not work during static destruction" argument.

I'm not sure that would address the issues, but then I'm just reporting the concerns. The key SG1 experts to answer "does the standard support a given example of detached threads during static destruction" aren't on this thread, so I encourage you to ask SG1 about examples -- we'll both be interested in the current answers.

I know you and I were both in at the beginning of it over a decade ago (aside: for those who don't know, Howard primarily is Mr. Std Thread for C++11!), but it hasn't gone away and the experts with the strongest recent opinions aren't here -- Hans, Detlef, Lawrence, and I know I'm forgetting one or two others.

[HowardHinnant]

Hans has said many times that detached threads can't be safely handled. For every time he has said it within my earshot, I show code that safely handles detached threads.

In my example every single read or write access to all done status flags is done under a lock of the mutex m. So I'm not understanding how there could be a race condition. Only one thread at a time has permission to read or write these flags.

Oh, except line 65 where the done flags are constructed. They are constructed outside the protection of the mutex. This is also prior to any thread being created, much less any thread having access to the flags. Therefore line 65 need not be protected by m.

[jwakely]

I took a good look at Howard's code and couldn't see any races. The threads exit after unlocking the mutex, so don't access anything after the cv.notify_all() calls. The main thread won't exit until the other threads have at least set their done flag and released the mutex (so the condition variable can relock it). It's possible the worker threads will still be in the process of exiting (after unlocking the mutex, before actually stopping running) when main returns, but the other threads don't access any globals at that point.

[HowardHinnant]

Right. This is a good example of a case where it is important to do the cv notify prior to unlocking the mutex. There is no way for main to act on spurious wake up and notice that the thread's status has been set to indicate that the thread has ended, because main can't get the mutex lock. The very last thing the thread touches before exiting is unlocking the mutex. From then on it is the std::lib's responsibility to clean up the thread if the program ends before the thread is done exiting.

Had the mutex been unlocked, and then the cv notified, it would be theoretically possible for main to spuriously wake up and destruct the cv before the thread called notify_all on it, resulting in UB.

[HowardHinnant]

Now that I think of it, THAT would be a good guideline!

When notifying a condition_variable, default to doing it before you unlock the mutex instead of after. Rationale: It is never wrong to unlock after the notify, and it is sometimes wrong to unlock before the notify. And there is no performance penalty in unlocking after, as good implementations take this coding pattern into account.

[hsutter]

Howard, thanks for the correction -- my "quick look" was too quick and I missed the careful use of the mutex and cv.

I think we're agreeing not to teach detaching as a default design choice.

I like your suggested condition_variable guideline, and I'll raise you one: I've long wished we had something like a "waitable_mutex" or "notifiable_mutex" abstraction in the standard library that encapsulated a mutex and an associated condition variable, so code that uses cv's in the most common patterns (all uses of the cv are supposed to be used with the same mutex) could avoid the need to remember to pass cv.wait() a lock to the right mutex. If you have a suggestion along that line it would be welcome!

[hsutter]

@MikeGitb: We have half of it with the wait(lk, pred). I'd like to fully automate the pattern of

{
    unique_lock hold{mut};
    while ( ! /* the protected state is what we are waiting for */ )
        cv.wait(hold); // unlock and reacquire
    /* main work */
}

With the predicate form, we have this much:

{
    unique_lock hold{mut};
    cv.wait(hold, []{ return /* the protected state is what we are waiting for */; });
    /* main work */
}

That's not too bad. But I was thinking of a way to avoid repeating cv and hold separately there, to avoid the opportunity to accidentally pass to cv.wait a lock that actually has locked a different mutex. Something like:

{
    conditional_lock hold{mutwithcv, []{ return /* the protected state is what we are waiting for */; }};
    /* main work */
}

so that the mutex and cv are bundled and we can't make the mistake of using the wrong one. This doesn't cover all cv cases, but it covers common ones. But I admit I haven't tried very hard to define this (or look for it -- I'd love to be told to just use something that already exists that I should already know about but don't).

[jwakely]

Building that on top of Olivier's http://wg21.link/p0126 would be optimal, but it's still going through redesign in SG1.

[jwakely]

@HowardHinnant

When notifying a condition_variable, default to doing it before you unlock the mutex instead of after.

I'll make a note to include it in the CV guidelines, writing them up has been on my TODO list for ages

[MikeGitb]

@hsutter: Sorry, I deleted my original post - I thought no one had seen it ;)

I made the mistake of only thinking about the signaling part, for which I still think a semaphore would be the best solution in 99% of the cases.
However, I ignored the fact that you very often need to modify some shared data right after the thread is woken up or right before the signaling anyway - just as you hinted in your example. With semaphores instead of CV+lock or your proposed mechanism this would obviously require an additional mutex or an otherwise threadsafe (e.g. lock-free) data structure.

[BjarneStroustrup]

I hope I have resolved this one. It's obviously tricky. Please review carefully.
raii_thread is now joining_thread and detached_thread is gone.
There is a "don't detach()" rule is there together with a recommendation to use scope to control lifetime of threads
Most rules about std::thread are gone, but I acknowledge that std::thread isn't going away (and joining_thread is a std::thread)

[ytimenkov]

I see you've discussed a lot and there are different views, but one major issue with joining_thread is that when it's a member of some object and the object is being destroyed from within thread (e.g. via shared_ptr), this usually leads to deadlock.
This is likely pattern used in 'worker' or 'event loop' classes, where class has internal thread(s) and interface to accept and dispatch messages, usually with shared ownership.

Other N cents:

Therefore I saw quite opposite recommendation: use only detached thread. Besides the system does proper cleanup it makes programmer think not in term of 'threads' but rather about 'communication'.

Some reasoning:

1. thread itself is not a real resource which must be handled like open file. At least in posix and windows threads system takes care of resources after return from function.
2. join() provides very limited synchronization and signaling, better to use mutex and CV.

Guideline give example of heartbeat() function and reason that 'detach makes it harder to monitor and communicate with the detached thread'. Fundamental problem I see here is that you don't communicate with thread, you communicate with object. For heartbeat there should be a Heartbeat_monitor class with detached thread and bool is_alive() const function which you communicate through.

I've looked at examples in Guidelines and it feels that wherever thread is used with plain function and then joined, the CP.4 rule and std::async() should be used instead.
Actually, what is the benefit of joining_thread over std::async?

Note on shared ownership: I find it very useful because this nicely solves 2 issues: 'CP.22 Never call unknown code while holding a lock' and memory management. So I had shared_ptr also attached to every callback to be dispatched, which kept the object alive while callback was in the queue or being executed, thus message loop could be safely destroyed even from thread it owns, e.g. if callback releases last 'public' reference to the message loop.