Make data_sink destruction and cleanup logic safer
- Dominant language
- C++
- Stars
- 9.8k
- Forks
- 1.1k
- Avg merge
- 3d 6m
- Merged PRs (30d)
- 278
Description
The `data_sink` class defines [a virtual destructor](https://github.com/rapidsai/cudf/blob/6e7bd1c812e394af838fb9c575af26e5686cfaed/cpp/include/cudf/io/data_sink.hpp#L104) with a default implementation that is a no-op along with [a pure virtual `flush` method](https://github.com/rapidsai/cudf/blob/6e7bd1c812e394af838fb9c575af26e5686cfaed/cpp/include/cudf/io/data_sink.hpp#L200). The current model assumes that subclasses will override the destructor and call their own flush method. For example, [the `file_sink` class overrides `flush`](https://github.com/rapidsai/cudf/blob/6e7bd1c812e394af838fb9c575af26e5686cfaed/cpp/src/io/utilities/data_sink.cpp#L52) and then [defines its destructor as calling that method](https://github.com/rapidsai/cudf/blob/6e7bd1c812e394af838fb9c575af26e5686cfaed/cpp/src/io/utilities/data_sink.cpp#L44).
While the code works if this pattern is followed, it has some very sharp edges in its current form. In an ideal world the parent `data_sink` class would define a non-virtual destructor that calls the child class's implementation of `flush`. However, that option is not possible because [polymorphic dispatch does not occur in constructors and destructors](https://isocpp.org/wiki/faq/strange-inheritance#calling-virtuals-from-ctors) (for good reasons). Therefore, we should come up with a solution that does not suffer from this limitation.
The best option that I can think of is to use CRTP to define the classes. The following should work (and as a bonus we could also replace other virtual function calls in `data_sink` with CRTP if any of them are performance-critical; we don't have to, though):
```c++
template
class data_sink {
~data_sink() { static_cast(this)->flush(); }
virtual void flush() = 0;
}
class file_sink : public data_sink {
void flush() override { /* implementation */ }
}
```
I am also open to other suggestions. The more explicit options I can think of would be using factory methods or explicit teardown functions before the object is destroyed, but those add more boilerplate and put the onus on the caller, which I like less than the status quo.
Contributor guide
Assessment
This issue has not been assessed yet.