26 ms·

Why an entirely new class? Why not just pass a constructor argument to specify joining behavior? It's not like there's any reason to believe this is the only th

Why an entirely new class? Why not just pass a constructor argument to specify joining behavior? It's not like there's any reason to believe this is the only thing we'll ever want to customize for a thread.

gpderetta 6y ago
I don't think making blocking on destruction conditional is a good thing. For example futures returned by std::async block on destruciton as opposed to other futures (even if they have the same type). This is generally considered a mistake as the future behaves very differently depending on how it was created and it can cause issues (i.e. deadlocks) for code that did not expect the blocking behaviour. As an aside I believe that all kind of blocking in a destructor (and thus jthread) is generally a mistake. In order of preference I believe that: - enforce explicit join, abort or detach via the type system (hard to do in C++) - abort the async computation - abort the whole program (i.e. assert) - detach the async computation are all preferable to blocking. I think I'm in the minority though.
- dataflow 6y ago
  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).
  - gpderetta 6y ago
    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.
    - OskarS 6y ago
      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.
      - gpderetta 6y ago
        > 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.
        dataflow 6y ago
  - simiones 6y ago
    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).
    - gpderetta 6y ago
      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.
    - dataflow 6y ago
      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.
jrimbault 6y ago
In my opinion, when you have an internal boolean flag for two behaviors, the interface you should probably expose isn't the flag in one function, but two different functions.
- dataflow 6y ago
  So like std::ios_base::fmtflags should actually be 19+ different functions and/or classes?
  - criddell 6y ago
    That isn't a boolean flag.
    - dataflow 6y ago
      Each bit is a Boolean flag (hex, scientific, etc.)... they're just packed into an integer. (Nobody said you have to use N bools for N different flags...)
      - eska 6y ago
        So does this API look well-designed to you?
- MaxBarraclough 6y ago
  edit I see I'm late, dataflow already made this point. I'll leave this here anyway. This rule of thumb is often useful but there are many exceptions. As an example, the internal behaviour of this OpenCL function [0] is presumably quite different depending on which bits of the second parameter are set, but it's still a good design as all possible behaviours of the function are still closely related as far as the user is concerned. [0] https://www.khronos.org/registry/OpenCL/sdk/1.2/docs/man/xhtml/clCreateBuffer.html https://www.khronos.org/registry/OpenCL/sdk/1.2/docs/man/xht...
  - jrimbault 6y ago
throwaway912598 6y ago
The main reason is so that the type system knows which thread is which and can handle it differently. They may still inherit from the same implementation and do what you suggest though.
- dataflow 6y ago
  > The main reason is so that the type system knows which thread is which and can handle it differently. Why is this useful in this case, is my question. You can try to put lots of things into the type system that aren't, but that doesn't automatically make them better. In fact I'd argue it makes things worse here, e.g., now every container will be instantiated twice depending on whether you use jthreads or threads, almost doubling the amount of work the compiler has to do for them.
  - throwaway912598 6y ago
    It's useful because you may be able to make compile-time decisions on the basis of the type of thread you're dealing with. What those are, I don't know. As for instantiations, an astute library developer will be able to do the right thing. It's not that hard.
  - gpderetta 6y ago
    say you have function that returns a thread object (which is perfectly fine as std::thread is moveable). The caller cannot assume that the thread will join on destruction and needs to conservatively join or detach explicitly anyway. Re container, they will only be instantiated twice if you actually use both std::jthread and std::thread of course. The idea (which I disagree) is that jthread is the better solution and std::thread is sort of deprecated.
    - dataflow 6y ago
      > The caller cannot assume that the thread will join on destruction and needs to conservatively join or detach explicitly anyway. This seems a little backwards to me? If the thread should join on destruction then that flag should be set. If not then it should be clear. I'd argue that flag should probably be set in general (and it seems you might feel the same way), but if you have a reason to clear it, then you're just fighting your own program by ignoring it. > Re container, they will only be instantiated twice if you actually use both std::jthread and std::thread of course. The idea (which I disagree) is that jthread is the better solution and std::thread is sort of deprecated. Good point!
fpoling 6y ago
Joinable thread and non-joinable one are very different in usage. It is rather appropriate to use a different class.
- masklinn 6y ago
  std::thread are already joinable (and that's their default state), and jthreads can be detached. The one difference between std::thread and jthread is that std::thread terminates on destruction, while jthread joins. That could easily be a ctor parameter (as well as "detach on destruction" as a third option).
- dataflow 6y ago
  > Joinable thread and non-joinable one are very different in usage. What is the usage of a "non-joinable" thread in the first place? I mentioned in a comment that that concept itself seems like a code smell to me, and certainly not something that should be the default. See here to read why: https://news.ycombinator.com/item?id=26142754 https://news.ycombinator.com/item?id=26142754
usefulcat 6y ago
Because std::jthread adds new methods as well. If you don't use a different class, then you end up with a weird situation where you still have to decide what get_stop_source() and get_stop_token() should do when the new constructor argument is omitted. That's definitely less clean, IMO.
- dataflow 6y ago
  I mean, you can just throw an exception or leave it undefined when the state isn't what you expect... isn't that a normal thing with any nontrivial class? e.g., what should unique_lock do if you call lock() when you passed adopt_lock_t to the constructor? What should unique_ptr do if you call operator->() when you passed null to the constructor? What should fstream do if you call open() when you already passed a file name to the constructor? etc.

gpderetta 6y ago

I don't think making blocking on destruction conditional is a good thing. For example futures returned by std::async block on destruciton as opposed to other futures (even if they have the same type). This is generally considered a mistake as the future behaves very differently depending on how it was created and it can cause issues (i.e. deadlocks) for code that did not expect the blocking behaviour. As an aside I believe that all kind of blocking in a destructor (and thus jthread) is generally a mistake. In order of preference I believe that: - enforce explicit join, abort or detach via the type system (hard to do in C++) - abort the async computation - abort the whole program (i.e. assert) - detach the async computation are all preferable to blocking. I think I'm in the minority though.

dataflow 6y ago
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).
- gpderetta 6y ago
  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.
  - OskarS 6y ago
    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.
    - gpderetta 6y ago
      > 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.
      - dataflow 6y ago
- simiones 6y ago
  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).
  - gpderetta 6y ago
    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.
  - dataflow 6y ago
    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.

jrimbault 6y ago

In my opinion, when you have an internal boolean flag for two behaviors, the interface you should probably expose isn't the flag in one function, but two different functions.

dataflow 6y ago
So like std::ios_base::fmtflags should actually be 19+ different functions and/or classes?
- criddell 6y ago
  That isn't a boolean flag.
  - dataflow 6y ago
    Each bit is a Boolean flag (hex, scientific, etc.)... they're just packed into an integer. (Nobody said you have to use N bools for N different flags...)
    - eska 6y ago
      So does this API look well-designed to you?
MaxBarraclough 6y ago
edit I see I'm late, dataflow already made this point. I'll leave this here anyway. This rule of thumb is often useful but there are many exceptions. As an example, the internal behaviour of this OpenCL function [0] is presumably quite different depending on which bits of the second parameter are set, but it's still a good design as all possible behaviours of the function are still closely related as far as the user is concerned. [0] https://www.khronos.org/registry/OpenCL/sdk/1.2/docs/man/xhtml/clCreateBuffer.html https://www.khronos.org/registry/OpenCL/sdk/1.2/docs/man/xht...
- jrimbault 6y ago

throwaway912598 6y ago

The main reason is so that the type system knows which thread is which and can handle it differently. They may still inherit from the same implementation and do what you suggest though.

dataflow 6y ago
> The main reason is so that the type system knows which thread is which and can handle it differently. Why is this useful in this case, is my question. You can try to put lots of things into the type system that aren't, but that doesn't automatically make them better. In fact I'd argue it makes things worse here, e.g., now every container will be instantiated twice depending on whether you use jthreads or threads, almost doubling the amount of work the compiler has to do for them.
- throwaway912598 6y ago
  It's useful because you may be able to make compile-time decisions on the basis of the type of thread you're dealing with. What those are, I don't know. As for instantiations, an astute library developer will be able to do the right thing. It's not that hard.
- gpderetta 6y ago
  say you have function that returns a thread object (which is perfectly fine as std::thread is moveable). The caller cannot assume that the thread will join on destruction and needs to conservatively join or detach explicitly anyway. Re container, they will only be instantiated twice if you actually use both std::jthread and std::thread of course. The idea (which I disagree) is that jthread is the better solution and std::thread is sort of deprecated.
  - dataflow 6y ago
    > The caller cannot assume that the thread will join on destruction and needs to conservatively join or detach explicitly anyway. This seems a little backwards to me? If the thread should join on destruction then that flag should be set. If not then it should be clear. I'd argue that flag should probably be set in general (and it seems you might feel the same way), but if you have a reason to clear it, then you're just fighting your own program by ignoring it. > Re container, they will only be instantiated twice if you actually use both std::jthread and std::thread of course. The idea (which I disagree) is that jthread is the better solution and std::thread is sort of deprecated. Good point!

fpoling 6y ago

Joinable thread and non-joinable one are very different in usage. It is rather appropriate to use a different class.

masklinn 6y ago
std::thread are already joinable (and that's their default state), and jthreads can be detached. The one difference between std::thread and jthread is that std::thread terminates on destruction, while jthread joins. That could easily be a ctor parameter (as well as "detach on destruction" as a third option).
dataflow 6y ago
> Joinable thread and non-joinable one are very different in usage. What is the usage of a "non-joinable" thread in the first place? I mentioned in a comment that that concept itself seems like a code smell to me, and certainly not something that should be the default. See here to read why: https://news.ycombinator.com/item?id=26142754 https://news.ycombinator.com/item?id=26142754

usefulcat 6y ago

Because std::jthread adds new methods as well. If you don't use a different class, then you end up with a weird situation where you still have to decide what get_stop_source() and get_stop_token() should do when the new constructor argument is omitted. That's definitely less clean, IMO.

dataflow 6y ago
I mean, you can just throw an exception or leave it undefined when the state isn't what you expect... isn't that a normal thing with any nontrivial class? e.g., what should unique_lock do if you call lock() when you passed adopt_lock_t to the constructor? What should unique_ptr do if you call operator->() when you passed null to the constructor? What should fstream do if you call open() when you already passed a file name to the constructor? etc.