NVIDIA / NVIDIA/cudf

Make data_sink destruction and cleanup logic safer

Open
#19,168 3 comments 0 reactions 0 assignees View on GitHub
cuIO improvement libcudf
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.