acdemiralp / gl

Header-only C++17 wrapper for OpenGL 4.6 Core Profile.
Boost Software License 1.0
157 stars 11 forks source link

Resource leak on std::move to managed buffer. #11

Closed Megaxela closed 6 years ago

Megaxela commented 6 years ago

If move one managed buffer (gl::vertex_buffer, gl::buffer, gl::texture etc.) to another managed buffer, target data will be leaked. (No glDeleteBuffers and etc. in move operator. Target Id will be lost.)

acdemiralp commented 6 years ago

Hey Alex, the ids should be setting to invalid_id on move see buffer for example:

buffer::buffer (      buffer&& temp) noexcept : id_(std::move(temp.id_)), managed_(std::move(temp.managed_))
{
#ifdef GL_CUDA_INTEROP_SUPPORT
  resource_ = std::move(temp.resource_);
#endif

  temp.id_       = invalid_id;
  temp.managed_  = false;
#ifdef GL_CUDA_INTEROP_SUPPORT
  temp.resource_ = nullptr;
#endif
}
acdemiralp commented 6 years ago

Ahh, after seeing the MR I see what you mean. The destructor is called upon the moved object after the move constructor. Which has invalidated the id, hence no deletion will occur.

There is no leak, and this is intentional, in order to preserve move semantics and make them identical for the CPU and the GPU. You do not reallocate but transfer the GPU reference to the new object. Note that the default constructor is not called within the move constructor, hence move does not allocate any GPU resource at all, it just transfers the existing resource onto itself.

Please correct me if I am wrong.

Megaxela commented 6 years ago

Yeah, sure, you right for move constructors, but what if I will create managed gl::buffer (with default constructor, for example) and then move another manager buffer into?

gl::buffer initialBuffer; // It's managed (glCreateBuffer was called)
gl::buffer secondBuffer; // It's managed (glCreateBuffer was called)
...
initialBuffer = std::move(secondBuffer); // operator=(gl::buffer&&) operator called. Now initialBuffers managed data was leaked, because it was not freed.
acdemiralp commented 6 years ago

You are correct. I am figuring out a fix.

acdemiralp commented 6 years ago

Solved in 2f08cf91069c8c3f37b0335e17d15d23ecc6b69d , thank you very much for detecting and fixing.