[scope] A few questions
Hi, I have a few questions regarding the scope library currently being reviewed: * scope_exit is not default constructible. But it can be released and is movable, so a default constructed scope_exit state makes sense (this is the same as a moved-from scope_exit). I find this a rather strange design choice. What is the rationale behind this? There's even a section on the documentation on how to circumvent this: https://lastique.github.io/scope/libs/scope/doc/html/scope/scope_guards.html... using cleanup_func_t = std::function< void() >; // Create an inactive scope guard first, since the cleanup function is not set yet boost::scope::scope_exit< cleanup_func_t > cleanup(cleanup_func_t(), false); Why can't i just write? boost::scope::scope_exit< std::function< void() > > cleanup; * the documentation says little about memory allocation. I finally found the answer i was looking for in the boost.ScopeExit comparison: scope_xxx objects do not do any heap allocations themselves. Is that a guarantee of the library? Such a guarantee is needed in some code i work with, i need to know if it is a feature of the library that i can rely upon, and not just an optimization that could go away in a further release. * unique_fd is a resource wrapper for files. But on all major systems, closing a file can fail and return an error. It may well be that the file is not completly written correctly, and this is reported not in the write call, but in the final close call. unique_fd silently drops the return value of close. I'm not sure we should encourage this behaviour. Another issue raises when we look at the example code: void write_string(std::string const& filename, std::string const& str) { // Create a unique resource for a file boost::scope::unique_resource< int, fd_deleter > file(open(filename.c_str(), O_CREAT | O_WRONLY)); if (file.get() < 0) { int err = errno; throw std::system_error(err, std::generic_category(), "Failed to open file " + filename); } // Use it to write data to the file const char* p = str.data(); std::size_t size_left = str.size(); while (size_left > 0) { ssize_t written = write(file.get(), p, size_left); if (written < 0) { int err = errno; if (err == EINTR) continue; throw std::system_error(err, std::generic_category(), "Failed to write data"); } p += written; size_left -= written; } } In case of a failure, what we got is an half-baked file, which we know nothing about. A proper write_string function should at least try to do its cleanup and delete the file in case of error. But that defeats the purpose of unique_resource... In my experience, automatically closing file handles is not error handling. But it has its use cases, maybe it would be better demonstrated by an example like: bool compare_files(std::string const& filename1, std::string const& filename2) { boost::scope::unique_resource< int, fd_deleter > file1(open(filename.c_str(), O_CREAT | O_WRONLY)); if (file1.get() < 0) { int err = errno; throw std::system_error(err, std::generic_category(), "Failed to open file " + filename1); } boost::scope::unique_resource< int, fd_deleter > file2(open(filename.c_str(), O_CREAT | O_WRONLY)); if (file2.get() < 0) { int err = errno; throw std::system_error(err, std::generic_category(), "Failed to open file " + filename2); } // do the comparison } Regards, Julien
On 11/30/23 17:54, Julien Blanc via Boost wrote:
Hi,
I have a few questions regarding the scope library currently being reviewed:
* scope_exit is not default constructible. But it can be released and is movable, so a default constructed scope_exit state makes sense (this is the same as a moved-from scope_exit). I find this a rather strange design choice. What is the rationale behind this?
Most of the time you construct a scope guard with some context, which means the action function object needs to be provided to the scope guard constructor. Actions with no data are pretty rare, so I didn't think a default-constructible scope guard would make sense. Also, the argument is needed for template argument deduction, whether through C++17 CTAD or via a factory function. I can add support for default construcion, if that is considered useful by the community.
There's even a section on the documentation on how to circumvent this: https://lastique.github.io/scope/libs/scope/doc/html/scope/scope_guards.html...
using cleanup_func_t = std::function< void() >; // Create an inactive scope guard first, since the cleanup function is not set yet boost::scope::scope_exit< cleanup_func_t > cleanup(cleanup_func_t(), false);
Why can't i just write?
boost::scope::scope_exit< std::function< void() > > cleanup;
The example with std::function is not really the best practice, as the section follows on after that example. You should generally avoid it, as it may throw on construction/assignment and therefore break your exception safety.
* the documentation says little about memory allocation. I finally found the answer i was looking for in the boost.ScopeExit comparison: scope_xxx objects do not do any heap allocations themselves. Is that a guarantee of the library? Such a guarantee is needed in some code i work with, i need to know if it is a feature of the library that i can rely upon, and not just an optimization that could go away in a further release.
There is a guarantee that scope guard construction won't throw, unless constructing one of the function objects throws. This implies that the scope guard itself doesn't do anything that may throw, which includes dynamic memory allocation.
* unique_fd is a resource wrapper for files. But on all major systems, closing a file can fail and return an error. It may well be that the file is not completly written correctly, and this is reported not in the write call, but in the final close call. unique_fd silently drops the return value of close. I'm not sure we should encourage this behaviour.
unique_resource is not designed to handle IO errors, it simply ensures you don't leak the resource. The actual correctness of resource management is still user's responsibility. If your code expects errors at the point of close, you should deal with them before calling close. That is, on a POSIX platform, call fsync/fdatasync before closing the file, and catch any errors there. For all intents and purposes, close should be regarded as a never failing deleter. Either it actually doesn't fail, or you don't care.
Another issue raises when we look at the example code:
void write_string(std::string const& filename, std::string const& str) { // Create a unique resource for a file boost::scope::unique_resource< int, fd_deleter > file(open(filename.c_str(), O_CREAT | O_WRONLY)); if (file.get() < 0) { int err = errno; throw std::system_error(err, std::generic_category(), "Failed to open file " + filename); }
// Use it to write data to the file const char* p = str.data(); std::size_t size_left = str.size(); while (size_left > 0) { ssize_t written = write(file.get(), p, size_left); if (written < 0) { int err = errno; if (err == EINTR) continue; throw std::system_error(err, std::generic_category(), "Failed to write data"); }
p += written; size_left -= written; } }
In case of a failure, what we got is an half-baked file, which we know nothing about. A proper write_string function should at least try to do its cleanup and delete the file in case of error.
The purpose of that example is not to provide a meaningful and bulletproof implementation of a string writing routine, for whatever definition of correctness. It is a simplified code sample that shows how to use unique_resource. Whether write_string is supposed to leave the half-written file, delete it or do something else with it is entirely irrelevant.
But that defeats the purpose of unique_resource... In my experience, automatically closing file handles is not error handling. But it has its use cases, maybe it would be better demonstrated by an example like:
bool compare_files(std::string const& filename1, std::string const& filename2) { boost::scope::unique_resource< int, fd_deleter > file1(open(filename.c_str(), O_CREAT | O_WRONLY)); if (file1.get() < 0) { int err = errno; throw std::system_error(err, std::generic_category(), "Failed to open file " + filename1); } boost::scope::unique_resource< int, fd_deleter > file2(open(filename.c_str(), O_CREAT | O_WRONLY)); if (file2.get() < 0) { int err = errno; throw std::system_error(err, std::generic_category(), "Failed to open file " + filename2); } // do the comparison }
Again, unique_resource is not about error handling, or IO in general for that matter. Its only purpose is to free the resource on destruction. However, I can update the example the way you suggest, to avoid the confusion. Thanks.
Le 2023-11-30 19:20, Andrey Semashev via Boost a écrit :
On 11/30/23 17:54, Julien Blanc via Boost wrote:
I can add support for default construcion, if that is considered useful by the community.
I think it does not hurt for scope_fail and scope_exit. We have scope_final for the simple case. I'm asking because i finally added it to the scope_exit-like we use in our code base. But we also have a type erased self-contained function object, which solves the issue below.
The example with std::function is not really the best practice, as the section follows on after that example. You should generally avoid it, as it may throw on construction/assignment and therefore break your exception safety.
Indeed. I fear, however, that people will just come up with this code, use it without understanding the implications. I think Andrzej made a very good point (https://lists.boost.org/Archives/boost//2022/05/253109.php) by saying that examples should be exempts from anti-patterns / bad code. I try to keep that in mind now when doing a review. To get back on the default construction, after more thoughts on this, it looks like a good rationale (can't guarantee that assignment will be nothrow) for *not* providing default construction. But doesn't that apply more generally? Isn't writing code like that: std::function<void()> getReleaseFunction(int fd) { return [fd]() { ::close(fd); }; } auto v = boost::scope::scope_exit(getReleaseFunction(fd)); something that should be completly forbidden, or at least strongly discouraged?
There is a guarantee that scope guard construction won't throw, unless constructing one of the function objects throws. This implies that the scope guard itself doesn't do anything that may throw, which includes dynamic memory allocation.
Thanks, this is the kind of definitive answer i was looking for. I could unfortunately not find it in the documentation, though. I think it deserves to be added (if it is present, then it probably should be emphasized more).
unique_resource is not designed to handle IO errors, it simply ensures you don't leak the resource. The actual correctness of resource management is still user's responsibility.
...
Again, unique_resource is not about error handling, or IO in general for that matter. Its only purpose is to free the resource on destruction.
However, I can update the example the way you suggest, to avoid the confusion.
I think there's indeed a dangerous confusion here, and it is exacerbated by the fact that both reside in the same library / namespace. scope_exit / scope_fail are helpers to write correct code. unique_fd is just about not leaking resources. That is indeed definitely not the same thing. Anyone who cares about file integrity won't use it when writing files (i have yet to see an raii design that works for file integrity). Regards, Julien
On 12/1/23 13:10, Julien Blanc wrote:
Le 2023-11-30 19:20, Andrey Semashev via Boost a écrit :
On 11/30/23 17:54, Julien Blanc via Boost wrote:
I can add support for default construcion, if that is considered useful by the community.
I think it does not hurt for scope_fail and scope_exit. We have scope_final for the simple case. I'm asking because i finally added it to the scope_exit-like we use in our code base. But we also have a type erased self-contained function object, which solves the issue below.
Ok, I will add support for default construction to scope guards. https://github.com/Lastique/scope/issues/11
The example with std::function is not really the best practice, as the section follows on after that example. You should generally avoid it, as it may throw on construction/assignment and therefore break your exception safety.
Indeed. I fear, however, that people will just come up with this code, use it without understanding the implications. I think Andrzej made a very good point (https://lists.boost.org/Archives/boost//2022/05/253109.php) by saying that examples should be exempts from anti-patterns / bad code. I try to keep that in mind now when doing a review.
I can't say I fully agree with Andrzej in that post. While it may look awkward at times that library documentation contains code samples that one shouldn't (or maybe even doesn't, in their right mind) write in actual real projects, sometimes this is just the most efficient way to demonstrate a library feature (such as that a function throws an exception) or warn about certain caveats or illustrate library misuse. Library documentation writers do rely on readers having a certain level of knowledge and intelligence, so that they are able to understand which examples are merely an illustration and not intended to be copy-pasted into real code. That said, maybe I could better articulate in text surrounding that example that such usage is the recommended way.
To get back on the default construction, after more thoughts on this, it looks like a good rationale (can't guarantee that assignment will be nothrow) for *not* providing default construction. But doesn't that apply more generally? Isn't writing code like that:
std::function<void()> getReleaseFunction(int fd) { return [fd]() { ::close(fd); }; }
auto v = boost::scope::scope_exit(getReleaseFunction(fd));
something that should be completly forbidden, or at least strongly discouraged?
Although using std::function has the caveats we've already discussed, I don't think strictly forbidding it would be the right thing to do. There may be uses where std::function doesn't affect the correctness of the surrounding code. So I prefer to make the user aware of the caveats and let him decide what's important to him. Besides, there isn't an efficient way to prevent the user from this issue. Sure, I could specialize scope guards on std::function (at the cost of including <functional>, which would penalize every user, even those who never wanted to use std::function to begin with), but it doesn't prevent users from using boost::function or any other equivalent.
There is a guarantee that scope guard construction won't throw, unless constructing one of the function objects throws. This implies that the scope guard itself doesn't do anything that may throw, which includes dynamic memory allocation.
Thanks, this is the kind of definitive answer i was looking for. I could unfortunately not find it in the documentation, though. I think it deserves to be added (if it is present, then it probably should be emphasized more).
It is part of the scope guard reference documentation, e.g.: https://lastique.github.io/scope/libs/scope/doc/html/boost/scope/scope_exit....
unique_resource is not designed to handle IO errors, it simply ensures you don't leak the resource. The actual correctness of resource management is still user's responsibility.
...
Again, unique_resource is not about error handling, or IO in general for that matter. Its only purpose is to free the resource on destruction.
However, I can update the example the way you suggest, to avoid the confusion.
I think there's indeed a dangerous confusion here, and it is exacerbated by the fact that both reside in the same library / namespace. scope_exit / scope_fail are helpers to write correct code. unique_fd is just about not leaking resources. That is indeed definitely not the same thing. Anyone who cares about file integrity won't use it when writing files (i have yet to see an raii design that works for file integrity).
For reference, I've added an issue about updating the docs: https://github.com/Lastique/scope/issues/9 I wouldn't say scope guards are a tool for error handling, though. I mean, they could be used for error handling in some cases, but (a) it's not the only use case and (b) scope guards usually have a limited capacity for error handling, as the action is usually not allowed to throw. I would say, the primary use case of scope guards is implementing transactional behavior of the code. And to some extent that may include resource management and error handling.
On 12/1/23 17:42, Andrey Semashev wrote:
On 12/1/23 13:10, Julien Blanc wrote:
Le 2023-11-30 19:20, Andrey Semashev via Boost a écrit :
The example with std::function is not really the best practice, as the section follows on after that example. You should generally avoid it, as it may throw on construction/assignment and therefore break your exception safety.
Indeed. I fear, however, that people will just come up with this code, use it without understanding the implications. I think Andrzej made a very good point (https://lists.boost.org/Archives/boost//2022/05/253109.php) by saying that examples should be exempts from anti-patterns / bad code. I try to keep that in mind now when doing a review.
I can't say I fully agree with Andrzej in that post. While it may look awkward at times that library documentation contains code samples that one shouldn't (or maybe even doesn't, in their right mind) write in actual real projects, sometimes this is just the most efficient way to demonstrate a library feature (such as that a function throws an exception) or warn about certain caveats or illustrate library misuse. Library documentation writers do rely on readers having a certain level of knowledge and intelligence, so that they are able to understand which examples are merely an illustration and not intended to be copy-pasted into real code.
That said, maybe I could better articulate in text surrounding that example that such usage is the recommended way.
...is *not* the recommended way.
Le 2023-12-01 15:42, Andrey Semashev via Boost a écrit :
Besides, there isn't an efficient way to prevent the user from this issue. Sure, I could specialize scope guards on std::function (at the cost of including <functional>, which would penalize every user, even those who never wanted to use std::function to begin with), but it doesn't prevent users from using boost::function or any other equivalent.
Actually, i was thinking of something far more radical. Something like requiring is_trivially_copyable<F>. From a theoric point of a view, it is clearly not the correct trait, but there's just no way to tell the compiler that we need the initializing expression to be noexcept. In practice, it does the job quite well at preventing bad usage, while still preventing not too much (in our code base, i can't recall of any case where this caused us trouble).
Thanks, this is the kind of definitive answer i was looking for. I could unfortunately not find it in the documentation, though. I think it deserves to be added (if it is present, then it probably should be emphasized more).
It is part of the scope guard reference documentation, e.g.:
https://lastique.github.io/scope/libs/scope/doc/html/boost/scope/scope_exit....
Thanks. Then i'll go for "it should be emphasized more". I was expecting to find such guarantee in the feature list of the library. Regards, Julien
On 12/5/23 12:17, Julien Blanc wrote:
Le 2023-12-01 15:42, Andrey Semashev via Boost a écrit :
Besides, there isn't an efficient way to prevent the user from this issue. Sure, I could specialize scope guards on std::function (at the cost of including <functional>, which would penalize every user, even those who never wanted to use std::function to begin with), but it doesn't prevent users from using boost::function or any other equivalent.
Actually, i was thinking of something far more radical. Something like requiring is_trivially_copyable<F>. From a theoric point of a view, it is clearly not the correct trait, but there's just no way to tell the compiler that we need the initializing expression to be noexcept. In practice, it does the job quite well at preventing bad usage, while still preventing not too much (in our code base, i can't recall of any case where this caused us trouble).
Requiring function objects to be trivially copyable is definitely going too far, as it prohibits binding most objects by value, even if such binding would not throw. For example, you wouldn't be able to bind smart-pointers, std::string or std::optional. It is possible to require the function object initialization to be noexcept, but requiring it would prohibit the use cases where throwing would be fine anyway.
Le 2023-12-05 12:24, Andrey Semashev via Boost a écrit :
On 12/5/23 12:17, Julien Blanc wrote:
Le 2023-12-01 15:42, Andrey Semashev via Boost a écrit :
Requiring function objects to be trivially copyable is definitely going too far, as it prohibits binding most objects by value, even if such binding would not throw. For example, you wouldn't be able to bind smart-pointers, std::string or std::optional.
That's true, but i would be curious about what's the occurrence of such cases in actual code. shared_ptrs (or anything refcounted) are the most obvious candidates. string looks like a misuse for me (i would not rely on sso). optional is trivially_copyable if the underlying type is, otherwise cf string. I'm curious about how much breakage adding such an assert causes in actual code, and which percentage of these breakage is actually a valid concern (ie, there was a potential problem that this assert detected). I guess the signal/noise ratio should either proves me completly wrong, or make a point for two distinct guards. Regards, Julien
participants (2)
-
Andrey Semashev -
Julien Blanc