boost-ext / di

C++14 Dependency Injection Library
https://boost-ext.github.io/di
1.13k stars 137 forks source link

boost::di::extension::shared_config doesn't work correctly with MSVC release build #512

Closed JulZimmermann closed 3 years ago

JulZimmermann commented 3 years ago

Hi, first off: Thank you for this amazing library!

Unfortunately, I found a bug with boost::di::extension::shared_config when using the MSVC compiler in release mode.

Take this minimal example:

#include <iostream>

#include "boost/di.hpp"
#include "boost/di/extension/scopes/shared.hpp"

class Bar {
public:
    int getI() const { return i; }

private:
    int i = 2;
};

class Foo {
public:
    Foo(std::shared_ptr<Bar> bar) : bar(std::move(bar)) {}
    std::shared_ptr<Bar> getBar() {  return bar; }

private:
    std::shared_ptr<Bar> bar;
};

class IBaz {
public:
    virtual ~IBaz() = default;
};

class Baz : public IBaz {
};

int main() {
    auto injector = boost::di::make_injector<boost::di::extension::shared_config>();

    auto baz = injector.create<std::shared_ptr<Baz>>();
    auto foo = injector.create<std::unique_ptr<Foo>>();

    std::cout << std::to_string(foo->getBar()->getI()) << std::endl;
}

This code runs fine with GCC in debug and release mode, as well as with MSVC in Debug mode and this program will print 2. A release build created with the MSVC compiler will print a random number because of some lifetime problem.

Some things I observed:

Expected Behavior

Output: 2

Actual Behavior

Undefined Behavior

Specifications

kanstantsin-chernik commented 3 years ago

Weird bug... You can try a workaround to bind types explicitly:

  auto injector = boost::di::make_injector<boost::di::extension::shared_config>(
      boost::di::bind<Bar>().in(boost::di::extension::shared)
  );
kanstantsin-chernik commented 3 years ago

The problem is these types have same type_ids:

type_id: 140700154865104 name: "struct Baz"
type_id: 140700154865104 name: "struct Bar"
kanstantsin-chernik commented 3 years ago

you can use this temporary instead of shared.hpp extension:

#pragma once

#include <memory>
#include <typeindex>
#include <unordered_map>
#if !defined(BOOST_DI_NOT_THREAD_SAFE)
#include <mutex>
#endif

#include "boost/di.hpp"
#include <string>

BOOST_DI_NAMESPACE_BEGIN
namespace extension {

namespace detail {
class shared {
 public:
  template <class, class T>
  class scope {
   public:
    scope() noexcept = default;

#if !defined(BOOST_DI_NOT_THREAD_SAFE)
    //<<lock mutex so that move will be synchronized>>
    explicit scope(scope&& other) noexcept : scope(std::move(other), std::lock_guard<std::mutex>(other.mutex_)) {}
    //<<synchronized move constructor>>
    scope(scope&& other, const std::lock_guard<std::mutex>&) noexcept : object_(std::move(other.object_)) {}
#endif

    template <class T_, class>
    using is_referable = typename wrappers::shared<shared, T>::template is_referable<T_>;

    template <class, class, class TProvider>
    static auto try_create(const TProvider& provider)
        -> decltype(wrappers::shared<shared, T>{std::shared_ptr<T>{provider.get()}});

    /**
     * `in(shared)` version
     */
    template <class, class, class TProvider>
    wrappers::shared<shared, T> create(const TProvider& provider) & {
      if (!object_) {
#if !defined(BOOST_DI_NOT_THREAD_SAFE)
        std::lock_guard<std::mutex> lock(mutex_);
        if (!object_)
#endif
          object_ = std::shared_ptr<T>{provider.get()};
      }
      return wrappers::shared<shared, T>{object_};
    }

    /**
     * Deduce scope version
     */
    template <class, class, class TProvider>
    wrappers::shared<shared, T> create(const TProvider& provider) && {
      auto& object = provider.cfg().template data<T>();
      if (!object) {
#if !defined(BOOST_DI_NOT_THREAD_SAFE)
        std::lock_guard<std::mutex> lock(mutex_);
        if (!object)
#endif
          object = std::shared_ptr<T>{provider.get()};
      }
      return wrappers::shared<shared, T>{std::static_pointer_cast<T>(object)};
    }

   private:
#if !defined(BOOST_DI_NOT_THREAD_SAFE)
    std::mutex mutex_;
#endif
    std::shared_ptr<T> object_;  /// used by `in(shared)`, otherwise destroyed immediately
  };
};
}  // namespace detail

static constexpr detail::shared shared{};

class shared_config : public di::config {
  template <class T>
  struct type {
    static void id() {}
  };
  template <class T>
  static auto type_id() {
    return reinterpret_cast<std::size_t>(&type<T>::id);
  }

 public:
  template <class T>
  struct scope_traits {
    using type = typename di::config::scope_traits<T>::type;
  };

  template <class T>
  struct scope_traits<T&> {
    using type = detail::shared;
  };

  template <class T>
  struct scope_traits<std::shared_ptr<T>> {
    using type = detail::shared;
  };

  template <class T>
  struct scope_traits<boost::shared_ptr<T>> {
    using type = detail::shared;
  };

  template <class T>
  struct scope_traits<std::weak_ptr<T>> {
    using type = detail::shared;
  };

  template <class T>
  auto& data() {
    return data_[typeid(T).name()];
  }

 private:
  std::unordered_map<std::string, std::shared_ptr<void>> data_{};
};

}  // namespace extension
BOOST_DI_NAMESPACE_END
kanstantsin-chernik commented 3 years ago

Hey @krzysztof-jusiak. Seems like MS reuses typeid for similar types. We cannot rely on id() any more.

krzysztof-jusiak commented 3 years ago

hmm, weird @kanstantsin-chernik. but we can always switch to simply use typeinfo/type_index :thinking:

kanstantsin-chernik commented 3 years ago

@krzysztof-jusiak Great idea! It actually works. Let me send a PR

JulZimmermann commented 3 years ago

Thank you so much for fixing this so quickly. I can confirm the problem is gone now,