felipecrv commented on code in PR #41876:
URL: https://github.com/apache/arrow/pull/41876#discussion_r1631354419
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -1724,9 +1728,9 @@ class ObjectOutputStream final : public io::OutputStream {
RETURN_NOT_OK(EnsureReadyToFlushFromClose());
- auto self =
std::dynamic_pointer_cast<ObjectOutputStream>(shared_from_this());
// Wait for in-progress uploads to finish (if async writes are enabled)
- return FlushAsync().Then([self]() { return
self->FinishPartUploadAfterFlush(); });
+ return FlushAsync().Then(
+ [self = Self()]() { return self->FinishPartUploadAfterFlush(); });
Review Comment:
+1. I proposed the use of `shared_from_this()` in this comment describing
issues with `CloseAsync`:
https://github.com/apache/arrow/pull/41232#issuecomment-2062514844
Other filesystems implement `CloseAsync` the same way.
##########
cpp/src/arrow/filesystem/s3fs.cc:
##########
@@ -1892,6 +1896,9 @@ class ObjectOutputStream final : public io::OutputStream {
}
// Notify completion
if (--state->parts_in_progress == 0) {
+ // GH-41862: avoid potential deadlock if the Future's callback is called
+ // with the mutex taken.
+ lock.unlock();
state->pending_parts_completed.MarkFinished(state->status);
Review Comment:
Now it's possible for this object to go from "Finished" to "not-finished".
There might be logic relying on the state machine converging to the finished
state and staying there.
I might do a full review of this class at some point.
--
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]
