4
\$\begingroup\$

AutoCancel is a class which wraps a Cancel (aka void()) function. The purpose of wrapping the function is to track when the function has gone out of scope, and call the wrapped function before being destroyed.

The below implementation is my attempt at achieving the intended goal.

Implementation:

#include <atomic>
#include <cassert>
#include <cstdint>
#include <functional>
#include <iostream>
#include <mutex>
#include <unordered_map>
#include <utility>

using Cancel = std::function<void()>;

class AutoCancel
{
  explicit AutoCancel(Cancel cancel, std::uint32_t id)
    : mCancel(std::move(cancel))
    , mId(id)
  {
    std::cout << "created id: " << id << std::endl;
  }

public:
  AutoCancel(const AutoCancel &other)
    : mCancel(other.mCancel)
    , mId(other.mId)
  {
    std::lock_guard lk(cancelablesMut);
    if (auto ref = cancelables.find(other.mId); ref != cancelables.end()) {
      ref->second++;
      std::cout << "Copy created via constructor id: " << mId << std::endl;
    }
  }
  AutoCancel &operator=(const AutoCancel &other)
  {
    if (mId != other.mId) {
      this->tryCancel();
      {
        std::lock_guard lk(cancelablesMut);
        if (auto ref = cancelables.find(other.mId); ref != cancelables.end()) {
          ref->second++;
        }
      }
      this->mCancel = other.mCancel;
      this->mId     = other.mId;
      std::cout << "Copy created via assignment id: " << mId << std::endl;
    }
    return *this;
  }
  AutoCancel(AutoCancel &&other) = default;
  AutoCancel &operator=(AutoCancel &&other)
  {
    this->tryCancel();
    this->mCancel = std::move(other.mCancel);
    this->mId     = std::move(other.mId);
    std::cout << "Move assignment id: " << mId << std::endl;
    return *this;
  }
  std::uint32_t id() const { return mId; }
  ~AutoCancel() { tryCancel(true); }
  void operator()()
  {
    std::lock_guard lk(cancelablesMut);
    if (hasCancel()) {
      doCancel();
    }
  }

protected:
  void doCancel(bool isDestroy = false)
  {
    std::cout << "destroy id: " << mId << std::endl;

    mCancel();
    mCancel = nullptr;
    if (!isDestroy) {
      std::cout << "untracking id: " << mId << std::endl;
      cancelables.erase(mId);
    }
    std::cout << "cancelled id: " << mId << std::endl;
    mId = 0;
  }

  bool tryCancel(bool isDestroy = false)
  {
    if (!hasCancel()) {
      return false;
    }

    std::lock_guard lk(cancelablesMut);
    auto const     &ref = cancelables.find(mId);
    if (ref == cancelables.end()) {
      // this may happen if the function was already called
      return false;
    }

    if (ref->second > 0) {
      ref->second--;
      return false;
    }

    assert(("No refs", cancelables.at(mId) == 0));
    doCancel(isDestroy);
    return true;
  }

  /**
   * @brief check if the cancel function is valid
   * @details if the cancel function is not valid, it means the object was
   * constructed from another object which was moved or already destroyed
   *
   * @return true if the cancel function is valid
   */
  bool hasCancel() const { return mCancel != nullptr; }

private:
  Cancel                                                         mCancel;
  std::uint32_t                                                  mId;
  inline static std::atomic_uint32_t                             ids { 1 };
  inline static std::unordered_map<std::uint32_t, std::uint32_t> cancelables;
  inline static std::mutex                                       cancelablesMut;

public:
  static auto wrap(Cancel cancel) -> AutoCancel
  {
    std::uint32_t    id = ids++;
    std::lock_guard  lk(cancelablesMut);
    cancelables[id] = 0;
    return AutoCancel(std::move(cancel), id);
  }
};

Test program:

auto createCancel(std::uint32_t tag)
{
  auto f = [tag]() {
    std::cout << "cancelling " << tag << std::endl;
  };
  return AutoCancel::wrap(std::move(f));
}

auto main() -> int
{
  AutoCancel track1(createCancel(1001));
  auto       track2 = createCancel(2002);
  track2            = std::move(track1);
  track2            = createCancel(3003);
  AutoCancel track3 = track2;
  track2();
  AutoCancel track4 = track3;
  AutoCancel track5 = track1;
  track5();
  return 0;
}

The expectation is that the messages that look like: cancelling XXXX should only appear as many times as there are calls to createCancel. Example run, using spdlog instead of std::cout:

[2024-04-10 12:22:48.745] [info] created id: 1
[2024-04-10 12:22:48.745] [info] created id: 2
[2024-04-10 12:22:48.745] [info] destroy id: 2
cancelling 2002
[2024-04-10 12:22:48.745] [info] untracking id: 2
[2024-04-10 12:22:48.745] [info] cancelled id: 2
[2024-04-10 12:22:48.745] [info] Move assignment id: 1
[2024-04-10 12:22:48.745] [info] created id: 3
[2024-04-10 12:22:48.745] [info] destroy id: 1
cancelling 1001
[2024-04-10 12:22:48.745] [info] untracking id: 1
[2024-04-10 12:22:48.745] [info] cancelled id: 1
[2024-04-10 12:22:48.745] [info] Move assignment id: 3
[2024-04-10 12:22:48.745] [info] Copy created via constructor id: 3
[2024-04-10 12:22:48.745] [info] destroy id: 3
cancelling 3003
[2024-04-10 12:22:48.745] [info] untracking id: 3
[2024-04-10 12:22:48.745] [info] cancelled id: 3

Aside from any instances of undefined behavior, I would also like feedback that pertains to my use of mutex and synchronization attempts.

One of the questions I didn't have an answer to was whether or not there is a guarantee for the order in which objects are destructed, i.e. can two objects be destructed at the same time? My use of locks was greatly influenced by lack of answer to this question, which is why I went with std::mutex instead of std::shared_mutex.

Test it online: https://coliru.stacked-crooked.com/a/d3d8054c2b750734

EDIT

I forgot to mention this in the original post, but another equally important goal of AutoCancel is to ensure that if any of the copies of the function executes, then the rest should be aware of this, and not call the function again. This is part of the reason why synchronization is global rather than local to each object.

\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Avoid relying on globally unique ids

A big problem with your class is that you need to provide a unique id, that the ids have to be managed, and that this is done using a global std::unordered_map. Ideally, you want to avoid having to specify ids at all. While you have a wrap() function that automatically assigns an id, this only works if you don't also manually provide ids, and there is also a problem if you allocate more than \$2^{32}\$ AutoCancel objects. You can avoid these issues by having AutoCancel allocate some object in memory to serve as a unique id and refcount. But that brings me to:

Consider replacing AutoCancel with std::shared_ptr

There is already a type in the standard library that can do exactly what you want, and that is std::shared_ptr. You can provide it with a deleter function, which will be called when the last reference to the shared object is gone. The pointer itself is not important, and can just be nullptr, and the deleter will still be called. However, you could use it to store an id if you wanted to. Here is your createCancel() rewritten to use std::shared_ptr:

auto createCancel(std::uint32_t tag) {
  auto cancel = [](std::uint32_t* tag) {
    std::cout << "cancelling " << *tag << "\n";
  };

  return std::shared_ptr(new std::uint32_t(tag), cancel);
};

And your main() function just needs a few adjustments to work, mainly more auto, and to explicitly decrease the refcount you can use the reset() member function:

int main()
{
  auto track1 = createCancel(1001);
  auto track2 = createCancel(2002);
  track2 = std::move(track1);
  track2 = createCancel(3003);
  auto track3 = track2;
  track2.reset();
  auto track4 = track3;
  auto track5 = track1;
  track5.reset();
}

Potential for deadlocks

I mentioned above that having a global map for ids is not ideal. Apart from uniqueness and performance issues, there is another problem. Consider that you want to register a cancellation function that does an operation on another AutoCancel object. Because the cancellation function is called with the global mutex cancelablesMut held, this will cause a deadlock.

What does an id of 0 mean?

In some functions you are treating an id of 0 as a special case of an already cancelled object, in others you don't. Consider this sequence with your createCancel():

auto foo = createCancel(0);
auto bar = createcancel(1);
bar();     // sets bar's id to 0
foo = bar; // ???

In the last line, nothing will happen; foo will continue to have an id of 0 and a recount of 1, but you would expect it to be replaced with something that has id 1 but a refcount of 0.

If you treat any id as valid when constructing new AutoCancel objects, then you must not internally treat some ids differently.

The move operator doesn't handle move from self

Consider:

auto foo = createCancel(1);
foo = std::move(foo);

I would expect the result to be that foo still has a refcount of 1. However, with your code it will be 0. You can fix this in various ways. One is to check if this == &other, another is to use the copy-and-swap idiom.

\$\endgroup\$
6
  • \$\begingroup\$ "Consider replacing AutoCancel with std::shared_ptr". You know, that did briefly cross my mind, but I didn't think it would have worked out the way I needed. One of the goals of the implementation is that whether or not the user gets a Cancel object or AutoCancel, they should both work the same. The problem with std::shared_ptr is that in order to "call" the underlying method, the user has to do cancel->operator()(), which is not very friendly to type. \$\endgroup\$
    – smac89
    Apr 10 at 23:07
  • \$\begingroup\$ As for your remark about id of 0, I think I might have edited the question after you started answering. The createCancel is not the one assigning an id to the callbacks. I renamed the parameter to tag from id to make that clearer. \$\endgroup\$
    – smac89
    Apr 10 at 23:08
  • \$\begingroup\$ Thanks for catching the bug with the move assignment operator. I should be doing the same check I did in the copy-assignment operator to determine if the same object is being moved to itself. \$\endgroup\$
    – smac89
    Apr 10 at 23:12
  • \$\begingroup\$ For the deadlocks issue, I think it can be mitigated by using an std::recursive_mutex. However, I think I have to find a way to avoid using mutexes and instead rely on std::shared_ptr \$\endgroup\$
    – smac89
    Apr 10 at 23:17
  • \$\begingroup\$ You can always create a class AutoCancel that internally uses std::shared_ptr for most of its operations. \$\endgroup\$
    – G. Sliepen
    Apr 11 at 5:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.