You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "andrei-21 (via GitHub)" <gi...@apache.org> on 2026/05/20 12:32:11 UTC
[PR] fix(dav-server): abort writer on write failure [opendal]
andrei-21 opened a new pull request, #7569:
URL: https://github.com/apache/opendal/pull/7569
# Which issue does this PR close?
Closes #7425.
# Rationale for this change
OpenDAL writers must be explicitly finalized with either `close()` or `abort()`. The dav-server integration previously adapted `Writer` into `FuturesAsyncWriter`, which only exposed `AsyncWrite` operations and did not allow `abort()` to be called when a write path failed before `close()`.
# What changes are included in this PR?
Keep the OpenDAL Writer in `OpendalFile` so failed write and close paths can abort the writer before returning an error. This prevents failed PUT operations from leaving the writer to be dropped without explicit termination.
# Are there any user-facing changes?
No.
# AI Usage Statement
Codex with gpt-5.5 model was used.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(dav-server): abort writer on write failure [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #7569:
URL: https://github.com/apache/opendal/pull/7569#discussion_r3604056004
##########
integrations/dav-server/src/file.rs:
##########
@@ -54,7 +54,7 @@ impl Debug for OpendalFile {
enum State {
Read(FuturesAsyncReader),
- Write(FuturesAsyncWriter),
+ Write(Option<Writer>),
Review Comment:
My two cents are to put state in use other than using Optional wrapper. This is an internal data structure.
It could be:
```
enum State {
Read(FuturesAsyncReader),
Writing(Writer),
Closed,
// could be a close failure or a flush failure. deliberate to use a failed state
Failed,
}
```
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(dav-server): abort writer on write failure [opendal]
Posted by "andrei-21 (via GitHub)" <gi...@apache.org>.
andrei-21 commented on code in PR #7569:
URL: https://github.com/apache/opendal/pull/7569#discussion_r3601966393
##########
integrations/dav-server/src/file.rs:
##########
@@ -54,7 +54,7 @@ impl Debug for OpendalFile {
enum State {
Read(FuturesAsyncReader),
- Write(FuturesAsyncWriter),
+ Write(Option<Writer>),
Review Comment:
`State::Write(None)` means:
> The file was opened in write mode, but its OpenDAL `Writer` is no longer usable.
It is set in two cases:
- `flush()` successfully called `Writer::close()`.
- A write or close failed, and `Writer::abort()` was called.
Afterward, further writes or flushes return `FsError::GeneralFailure`.
So `None` is a terminal-state sentinel—not an empty file or a writer that hasn’t been created. A clearer model would be:
```rust
enum State {
Read(FuturesAsyncReader),
Writing(Writer),
Closed,
Failed,
}
```
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(dav-server): abort writer on write failure [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #7569:
URL: https://github.com/apache/opendal/pull/7569#discussion_r3604056004
##########
integrations/dav-server/src/file.rs:
##########
@@ -54,7 +54,7 @@ impl Debug for OpendalFile {
enum State {
Read(FuturesAsyncReader),
- Write(FuturesAsyncWriter),
+ Write(Option<Writer>),
Review Comment:
My two cents are to put state in use other than using Optional wrapper. This is an internal data structure.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(dav-server): abort writer on write failure [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #7569:
URL: https://github.com/apache/opendal/pull/7569#discussion_r3597546745
##########
integrations/dav-server/src/file.rs:
##########
@@ -54,7 +54,7 @@ impl Debug for OpendalFile {
enum State {
Read(FuturesAsyncReader),
- Write(FuturesAsyncWriter),
+ Write(Option<Writer>),
Review Comment:
This is a strange design. What does a State::Write(None) mean?
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org