Re: [scope] A few questions

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2023-11-30 18:20:30

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#scope.scope_guards.runtime_defined
> 
> 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.