Hacker News Reader

Comment by dataflow | Hacker News Reader

Could you elaborate on why blocking in a thread's destructor is bad? You mention it is but don't really give any reason. Every time I use std::thread I find the need to wait on it in the destructor. In fact any non-joining thread is a code smell to me. If you just detach (or abort...) a thread, then there's simply no way to ensure that main() will return after the thread has finished—and you always want to make sure all threads finish their tasks and clean up properly before the process terminates. This requires you to wait on threads during app shutdown, which generally corresponds with destruction, which means you need to join on destruction. I actually don't understand why you would want it another way by default (if at all).

View on HN

I just think that blocking is an important enough effect that that needs to be accounted for and it should be explicit in code. Deadlocking is the obvious reason, but also the possibly large pauses are problematic.

I'm a big believer in crash-only software and I think that "clanup" at shutdown is code smell and a symptom of unreliable software [1]. Just call _Exit when the application is done.

[1] Cleaning up to decrease the noise for the benefit of a leak detector is useful though, but by experience, I'm not yet convinced that the effort is worth it.

Just last week I was debugging an issue with a library that used SQLite internally, and I noticed that whenever I shut down, it would leave all these "something.db-shm" and "something.db-wal" files around in addition to the "something.db" file. My hunch was the library wasn't properly deinitializing SQLite, and indeed that was the case, if you call sqlite3_close() properly, these files are deleted (they're essentially journal files for SQLite). The library was essentially doing what you're suggesting, ending without doing the proper cleanup.

Destruction, in general, is one of the most complex areas of software development in my opinion. You're right, blocking or throwing errors during destruction is terrible, but you really do have to genuinely clean things up when exiting, you can't leave messes behind like this. However, in addition to that: you can't ever count on destruction occuring, because it's entirely possible that your program crashes, the user kills the process, the computer runs out of battery (or someone janks the cable out) or some other random thing causes your process to exit prematurely.

You really need a lot of nuance in this area, and I don't agree with "cleanup at shutdown is a code smell". But you're certainly right that you can go too far in the other direction as well: destruction has to be fast, non-blocking, error-free, and never required.

> you can't ever count on destruction occuring, because it's entirely possible that your program crashes, the user kills the process, the computer runs out of battery (or someone janks the cable out) or some other random thing causes your process to exit prematurely.

Well yes, that's why you can't wait for shutdown time to cleanup your mess. In this case, either the files need to survive process exit or they can be unlinked as soon as they are created. Another option is to have very simple supervisor process whose only job is to cleanup.

For a DB like sqlite it does makes sense to have some amount of shutdown cleanup to close transactions and mark the log as stale and avoid any recovery on startup, but it should be considered an optimization and not be a requirement.

To make this short, I do in fact agree with your nuanced view. Pragmatism always trumps dogmatism in software engineering.

> In this case, either the files need to survive process exit or they can be unlinked as soon as they are created.

The latter literally isn't possible on Windows. And I'd argue it's a good thing because you shouldn't do that in this case, though either way you don't have that option anyway. The notion of having a supervisory process just to clean up your mess that you had every chance to clean up is just poor design. Supervisory processes are for when you can't do that and there's critical work left over, not for cleaning up random clutter you just didn't bother to try to handle at the correct time. Imagine if every program that used SQLite spawned an extra supervisory process? Now imagine if this was true for every program that needed temporary files as long as it was running? That's obviously just terrible. Your code needs to clean up after itself as much as it can, whether via RAII or otherwise, but you can't just throw out the notion of cleaning things up on shutdown (or any other point) altogether!

you do not have a chance to cleanup if you are application is killed anyway. So either don't make a mess, make sure that you can clean up no matter what or live with it.

> you do not have a chance to cleanup if you are application is killed anyway.

Then you live with it in that case. That doesn't mean you should litter when the application is terminating gracefully. You clean up on shutdown when you are able. I don't see what's contentious about this.

Yeah I'd argue these problems happen because people don't take advantage of RAII (maybe because they don't like it or their language doesn't support it), and then forget/neglect to do the proper cleanups at the correct time. You want to design things so that they encapsulate their correctness by design. For a thread, that means joining it on destruction (because that should happen in general). For a mutex, it means not unlocking it on destruction (because that signals another part of the code is actually incorrect). For lock guard, it means unlocking on destruction. For files, it means closing them on destruction. Etc. You can't just reduce everything to "always" or "never".

I think there are two schools of thought on this issue:

1. Blocking/unblocking is a major side effect, so it should be handled automatically so you don't forget about it

2. Blocking/unblocking is a major side effect, so it should be very prominent in the code so you don't miss it when reading, and it doesn't accidentally happen when you don't expect it

The C++ designers usually favor the first school, leading to designs like `std::lock_guard`. Many others, myself included, favor the second school, and would complain that the code marker that a method will block forever waiting for a thread to finish shouldn't be "}".

I would also say that in general blocking forever should not be the default - you should have to explicitly ask for it, not have it as a default. This could be achieved with a RAII design by passing the timeout parameter to the constructor, but that obscures the meaning of the } even more, especially if the thread ownership ever changes hands (a function receiving a std::unique_ptr<std::jthread> would wait an unknown amount of time on a }, for example).

The thing is, you talk about how blocking implicitly is bad, but I didn't say "it's good to block in the destructor" as a blanket rule either. I'm talking about this very particular case of blocking deserves to be in the destructor, for what I thought were fairly decent reasons that I spelled out. You haven't actually addressed my points about threads specifically and are just talking about blocking in a destructor in general, but the discussion/disagreement isn't about other cases. There are definitely times when I think it'd be wrong to block implicitly, but this just isn't one of them.

I think the arguments I presented do hold for thread joining as well as other cases: it's less clear from the code that this:

  int main(int argc, char[][]argv) {
    std::jthread t([](){
      volatile int a; 
      while(true) a=1; });
  }

will block forever, than it is for this:

  int main(int argc, char[][]argv) {
    std::jthread t([](){
      volatile int a; 
      while(true) a=1; });
    t.join();
  }
  //using the volatiles since while(true); is UB, nothing to do with threading

This is even more true for a class that has a std::jthread member.

I've also asserted that the api of std::jthread::join is not great, that it would have been pereferable to have a mandatory timeout parameter, so that someone calling it would be forced to think about an appropriate timeout, since blocking forever is rarely the right choice (of course, you would have a constant for blocking forever as well - this is not about preventing this behavior, just slightly discouraging it).

If you agree that blocking forever waiting for a thread to join is in itself something to be discouraged (not made impossible, but also not the default), then it also follows that having a destructor block at all should also be avoided in the std lib.

I do want to emphasize that I'm not claiming that there are no cases where this sort of behavior is useful, just that it is more rarely the best choice. I also believe in the idea that language/library design should nudge users into the right direction, so that the better use case is also the easier one to write.

> it's less clear from the code that this will block forever

Sure! That doesn't imply that's a bad thing. It's not like "clarity" is transitive. Do all your callers know when you're calling lock_guard() for example? No. Should they? No.

> than it is for this:

You actually sacrifice correctness here for "clarity". What happens if you return control flow returns to the caller before join() is called, whether normally (early return, etc.) or via an exception? Suddenly you have a thread that is out of your control, and the caller has absolutely no guarantees about any aspect of it. At best, your program might terminate in the middle of that thread's work, and wreck whatever it was doing (maybe it was working on some file or communicating with another system). At worst, your code was called in a loop and you actually bring down the whole system, e.g. by launching new threads before the old ones have terminated. It's a massive bug to return with rogue threads.

> If you agree that blocking forever waiting for a thread to join is in itself something to be discouraged (not made impossible, but also not the default), then it also follows that having a destructor block at all should also be avoided in the std lib.

What I'm saying is this isn't true. Blocking should be discouraged in most cases, but this case is specifically an exception to that. Just like how "calling 'delete' directly should be discouraged" has obvious exceptions like "except in the destructor of a smart pointer". And just like how "calling mutex::unlock() directly should be discourage" has obvious exceptions like "except in the destructor of a lock guard". Your reasoning simply doesn't follow, and for good reason.

Moreover, I also beg to differ on the notion that "every blocking operation should have a timeout"; that's true in certain scenarios such as when you're waiting for a component in a distributed system (like an event on another process or machine that might go down without guaranteed notification to you), but quite false in general. Just look at lock_guard... it has no timeout, and it's used all over the place. Moreover, any operation you do that involves a heap allocation or deallocation can block indefinitely with no timeout, and it's perfectly fine. And if you use Windows, look for example at how GetMessage() has no timeout, and note that it's used in pretty much every GUI program for messaging within threads, between threads, and between processes. I could cite more examples but hopefully this illustrates what I mean.

The sense I'm getting from your arguments is that you simply don't believe and/or trust in 'scoped' operations. I don't know how to convince you here, but if you scope your operations match destructors to constructors like they're supposed to be, some things that could go wrong simply won't go wrong. Because, more abstractly, this lets you decompose your program into a tree instead of a DAG, and all the niceness that comes with that just comes automatically.

Not the parent, but mine is not a theoretical argument. I work on an highly threaded application. Many components spawn threads, send messages to other components running on other threads and wait for messages or events from other components. When you have base classes and members all blocking in their destructors is very easy to end up with deadlocks or race conditions just by changing the order of members or base classes and things like that.

By having an explicit (asynchronous) shutdown phase this can be avoided.

There have been some talk in the committee about having async destructors. That might be the solution to the problem.

So let me see if I understand this in a simplified scenario. You're saying your code might be something like:

  class X { void inform_shutdown(); void run(); };
  class A { std::jthread t; };
  class B { X x; ~B() { x.inform_shutdown(); } };
  class C { A a; B b; };

and initially all is fine, because A's destructor only runs after B is destroyed, and thus messages are exchanged before ~A blocks on the thread, but then the code eventually involves to something like:

  class C { B b; A a; };

and suddenly now ~B can't send shutdown messages and hence the thread won't know to exit in the first place. Is this an example of what you're saying?

To me, it sounds like the underlying issue is something else here. It's hard to say without a concrete example, but if the above is accurate, it might be that a hidden dependency that needs to be made explicit. If X::run is running (say) an infinite work loop on its own thread, it is coupled to that thread in some fashion. For example, it might have some kind of message queue to pull from, and that queue belongs to the thread (i.e. you would put the message queue in A). That means X needs a dependency on A just as much as C does, meaning you'd want to hold std::shared_ptr<A> instead of A, and hence you won't have this blocking issue anymore because it will only occur when the last user of A is destroyed.

Obviously your scenario might have some differences from this, but it seems quite plausible to me that the problem might have a different solution than non-joining threads.

Yes, there are complex dataflow (ah!) dependencies between heterogeneous threads, often bidirectional. Using shared pointers would means trading potential deadlocks with potential leaks due to circular dependencies (I'm not saying it is worse, but it is a tradeoff).

I'd argue there's no tradeoff there. Cyclic references are also avoidable (and should be avoided) in general in my experience. How to achieve that again requires a specific example, as there's no one-size-fits-all-solution. It's great that we got here actually, because it demonstrates how higher level constraints that seem so rigid and impenetrable actually stem from simpler design considerations like these that are often disregarded in the beginning.

Oh, we solve it all right. We have single blocking point for business level code [1] at the top level of the program. Everything else is asynchronous and it is done by future combining.

[1] We also block on each thread executor event loop (at least those that are not spinning) of course, but that's hidden from the business level logic.

So, I guess I fall in the middle.

I have no issues with lock_guard. Also, it will unlock on destruction, which is not much of a problem. And having this sort of side effect at end of scope is consistent with languages having synchronized or atomic blocks.

I would also not have much of a problem with a join_guard that joins one or more thread on destruction.

I am a fan of RAII and both lock_guard and join_guard have a single job and if you use them it is pretty obvious what you want, you can opt in on them and you can delegate the job by passing them around.

My issue is with more complex objects blocking on destruction when it is not their primary purpose.

Reply on news.ycombinator.com