zjuwangg commented on code in PR #10111:
URL:
https://github.com/apache/incubator-gluten/pull/10111#discussion_r2204059410
##########
cpp/core/utils/ObjectStore.cc:
##########
@@ -37,23 +37,34 @@ gluten::ResourceMap<gluten::ObjectStore*>&
gluten::ObjectStore::stores() {
}
gluten::ObjectStore::~ObjectStore() {
- // destructing in reversed order (the last added object destructed first)
- const std::lock_guard<std::mutex> lock(mtx_);
- for (auto itr = aliveObjects_.rbegin(); itr != aliveObjects_.rend(); itr++) {
- const ResourceHandle handle = (*itr).first;
- const auto& info = (*itr).second;
- const std::string_view typeName = info.typeName;
- const size_t size = info.size;
- VLOG(2) << "Unclosed object ["
- << "Store ID: " << storeId_ << ", Resource handle ID: " << handle
<< ", TypeName: " << typeName
- << ", Size: " << size
- << "] is found when object store is closing. Gluten will"
- " destroy it automatically but it's recommended to manually
close"
- " the object through the Java closing API after use,"
- " to minimize peak memory pressure of the application.";
- store_.erase(handle);
+ std::vector<std::shared_ptr<void>> objects;
+ {
+ // destructing in reversed order (the last added object destructed first)
+ const std::lock_guard<std::mutex> lock(mtx_);
+ for (auto itr = aliveObjects_.rbegin(); itr != aliveObjects_.rend();
itr++) {
+ const ResourceHandle handle = (*itr).first;
+ const auto& info = (*itr).second;
+ const std::string_view typeName = info.typeName;
+ const size_t size = info.size;
+ VLOG(2) << "Unclosed object ["
+ << "Store ID: " << storeId_ << ", Resource handle ID: " <<
handle << ", TypeName: " << typeName
+ << ", Size: " << size
+ << "] is found when object store is closing. Gluten will"
+ " destroy it automatically but it's recommended to manually
close"
+ " the object through the Java closing API after use,"
+ " to minimize peak memory pressure of the application.";
+ // transfer ownership of the object to the temporary vector
+ objects.push_back(store_.lookup(handle));
+ store_.erase(handle);
+ }
+ stores().erase(storeId_);
+ }
+ // destructing objects outside the lock to avoid deadlock
+ for (auto& obj : objects) {
+ if (obj) {
+ obj.reset();
Review Comment:
You are right but in this situation there will be no problem for
ParquetFileWriter, which will ignore all exception during it's destructors as
https://github.com/facebookincubator/velox/blob/main/velox/dwio/parquet/writer/arrow/FileWriter.cpp#L646.
`In C++, destructors should generally not throw exceptions that are allowed
to escape the destructor. This is a crucial guideline for writing robust and
reliable C++ code`.
For other obj besides `ParquetFileWriter`, if `object not found` being
thrown we can fix it's destructors method, it's better to run in endless
deadLock loop.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
