You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "Kriskras99 (via GitHub)" <gi...@apache.org> on 2025/05/22 10:42:28 UTC
[PR] fix: flush Writer on drop [avro-rs]
Kriskras99 opened a new pull request, #199:
URL: https://github.com/apache/avro-rs/pull/199
This brings the writer in line with standard library types like `BufWriter` and `LineWriter`.
This is a breaking change when the inner writer is `&mut W` and used later in the scope without dropping the writer or calling `into_inner` (as seen in the fixed tests).
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g merged PR #199:
URL: https://github.com/apache/avro-rs/pull/199
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #199:
URL: https://github.com/apache/avro-rs/pull/199#discussion_r2115812717
##########
avro/tests/to_from_avro_datum_schemata.rs:
##########
@@ -110,6 +110,7 @@ fn test_avro_3683_multiple_schemata_writer_reader() -> TestResult {
let mut writer = Writer::with_schemata(schema_b, schemata.clone(), &mut output, Codec::Null);
writer.append(record.clone())?;
writer.flush()?;
+ let _ = writer.into_inner()?;
Review Comment:
What kind of magic happens here ?
Why calling `into_inner()` makes it possible to use `output` again ?
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "Kriskras99 (via GitHub)" <gi...@apache.org>.
Kriskras99 commented on PR #199:
URL: https://github.com/apache/avro-rs/pull/199#issuecomment-2922573680
Not sure why the CI is angry, I don't see a double get_ref or get_mut implementation. And I can't reproduce it in codespaces
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "Kriskras99 (via GitHub)" <gi...@apache.org>.
Kriskras99 commented on code in PR #199:
URL: https://github.com/apache/avro-rs/pull/199#discussion_r2115892399
##########
avro/tests/to_from_avro_datum_schemata.rs:
##########
@@ -110,6 +110,7 @@ fn test_avro_3683_multiple_schemata_writer_reader() -> TestResult {
let mut writer = Writer::with_schemata(schema_b, schemata.clone(), &mut output, Codec::Null);
writer.append(record.clone())?;
writer.flush()?;
+ let _ = writer.into_inner()?;
Review Comment:
`output` is giving as a mutable reference to `Writer` resulting in a `Writer<&mut Vec<u8>`.
Because `Writer` now has a Drop implementation, it lives until the end of the function and therefore requires the mutable reference to live until the end.
By calling `into_inner()` the mutable reference is "given back" to `output` and it can be used again.
Looking at it again, I would modifiy the code like this:
```rust
let mut writer = Writer::with_schemata(schema_b, schemata.clone(), output, Codec::Null);
writer.append(record.clone())?;
writer.flush()?;
let output = writer.into_inner()?;
```
which makes it a lot less magical.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #199:
URL: https://github.com/apache/avro-rs/pull/199#issuecomment-2923505349
Thank you once again, @Kriskras99 !
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "Kriskras99 (via GitHub)" <gi...@apache.org>.
Kriskras99 commented on PR #199:
URL: https://github.com/apache/avro-rs/pull/199#issuecomment-2922333742
> I do not see similar code in [LineWriter](https://doc.rust-lang.org/beta/src/std/io/buffered/linewriter.rs.html)
That's because LineWriter is just a wrapper around BufWriter and thus doesn't have to implement this.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #199:
URL: https://github.com/apache/avro-rs/pull/199#discussion_r2115939694
##########
avro/tests/to_from_avro_datum_schemata.rs:
##########
@@ -110,6 +110,7 @@ fn test_avro_3683_multiple_schemata_writer_reader() -> TestResult {
let mut writer = Writer::with_schemata(schema_b, schemata.clone(), &mut output, Codec::Null);
writer.append(record.clone())?;
writer.flush()?;
+ let _ = writer.into_inner()?;
Review Comment:
> now has a Drop implementation, it lives until the end of the function
Could you please point me to the Rust documentation confirming this ?
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "Kriskras99 (via GitHub)" <gi...@apache.org>.
Kriskras99 commented on PR #199:
URL: https://github.com/apache/avro-rs/pull/199#issuecomment-2919121172
Understandable, if you have any questions just ask. The implementation is based on `std::io::BufWriter`.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #199:
URL: https://github.com/apache/avro-rs/pull/199#issuecomment-2922183357
> This brings the writer in line with standard library types like `BufWriter` and `LineWriter`.
I do not see similar code in [LineWriter](https://doc.rust-lang.org/beta/src/std/io/buffered/linewriter.rs.html)
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #199:
URL: https://github.com/apache/avro-rs/pull/199#issuecomment-2919054276
I will have to do some research before merging this PR.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #199:
URL: https://github.com/apache/avro-rs/pull/199#discussion_r2115694582
##########
avro/src/writer.rs:
##########
@@ -348,7 +354,18 @@ impl<'a, W: Write> Writer<'a, W> {
pub fn into_inner(mut self) -> AvroResult<W> {
self.maybe_write_header()?;
self.flush()?;
- Ok(self.writer)
+
+ let mut this = ManuallyDrop::new(self);
+
+ // Extract every member that is not Copy and therefore should be dropped
+ let _resolved_schema = std::mem::take(&mut this.resolved_schema);
+ let _buffer = std::mem::take(&mut this.buffer);
+ let _user_metadata = std::mem::take(&mut this.user_metadata);
Review Comment:
The `into_inner` implementation doesn’t take `schemata`, so that field remains in `ManuallyDrop` and leaks memory. Consider adding `let _schemata = std::mem::take(&mut this.schemata);` before reading out the writer.
```suggestion
let _user_metadata = std::mem::take(&mut this.user_metadata);
let _schemata = std::mem::take(&mut this.schemata);
```
##########
avro/src/writer.rs:
##########
@@ -25,12 +25,18 @@ use crate::{
AvroResult, Codec, Error,
};
use serde::Serialize;
-use std::{collections::HashMap, io::Write, marker::PhantomData, ops::RangeInclusive};
+use std::{
+ collections::HashMap, io::Write, marker::PhantomData, mem::ManuallyDrop, ops::RangeInclusive,
+};
const DEFAULT_BLOCK_SIZE: usize = 16000;
const AVRO_OBJECT_HEADER: &[u8] = b"Obj\x01";
/// Main interface for writing Avro formatted values.
+///
+/// It is critical to call flush before `Writer<W>` is dropped. Though dropping will attempt to flush
Review Comment:
Update this comment: since `Writer` now auto-flushes in its `Drop` impl (ignoring errors), calling `flush` before drop is no longer strictly required for data integrity.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "Kriskras99 (via GitHub)" <gi...@apache.org>.
Kriskras99 commented on code in PR #199:
URL: https://github.com/apache/avro-rs/pull/199#discussion_r2115729686
##########
avro/src/writer.rs:
##########
@@ -25,12 +25,18 @@ use crate::{
AvroResult, Codec, Error,
};
use serde::Serialize;
-use std::{collections::HashMap, io::Write, marker::PhantomData, ops::RangeInclusive};
+use std::{
+ collections::HashMap, io::Write, marker::PhantomData, mem::ManuallyDrop, ops::RangeInclusive,
+};
const DEFAULT_BLOCK_SIZE: usize = 16000;
const AVRO_OBJECT_HEADER: &[u8] = b"Obj\x01";
/// Main interface for writing Avro formatted values.
+///
+/// It is critical to call flush before `Writer<W>` is dropped. Though dropping will attempt to flush
Review Comment:
It is still required for data integrity as errors are ignored.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "Kriskras99 (via GitHub)" <gi...@apache.org>.
Kriskras99 commented on code in PR #199:
URL: https://github.com/apache/avro-rs/pull/199#discussion_r2115732535
##########
avro/src/writer.rs:
##########
@@ -348,7 +354,18 @@ impl<'a, W: Write> Writer<'a, W> {
pub fn into_inner(mut self) -> AvroResult<W> {
self.maybe_write_header()?;
self.flush()?;
- Ok(self.writer)
+
+ let mut this = ManuallyDrop::new(self);
+
+ // Extract every member that is not Copy and therefore should be dropped
+ let _resolved_schema = std::mem::take(&mut this.resolved_schema);
+ let _buffer = std::mem::take(&mut this.buffer);
+ let _user_metadata = std::mem::take(&mut this.user_metadata);
Review Comment:
That field does not exist
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "Kriskras99 (via GitHub)" <gi...@apache.org>.
Kriskras99 commented on code in PR #199:
URL: https://github.com/apache/avro-rs/pull/199#discussion_r2116017028
##########
avro/tests/to_from_avro_datum_schemata.rs:
##########
@@ -110,6 +110,7 @@ fn test_avro_3683_multiple_schemata_writer_reader() -> TestResult {
let mut writer = Writer::with_schemata(schema_b, schemata.clone(), &mut output, Codec::Null);
writer.append(record.clone())?;
writer.flush()?;
+ let _ = writer.into_inner()?;
Review Comment:
See this playground example with `BufWriter`: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=c8318f0b348bf8be78536bd33b619b3e
If you look at the compiler error, it explains the issue. I'm not sure where this would be documented.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "Kriskras99 (via GitHub)" <gi...@apache.org>.
Kriskras99 commented on code in PR #199:
URL: https://github.com/apache/avro-rs/pull/199#discussion_r2115768913
##########
avro/src/writer.rs:
##########
@@ -348,7 +354,18 @@ impl<'a, W: Write> Writer<'a, W> {
pub fn into_inner(mut self) -> AvroResult<W> {
self.maybe_write_header()?;
self.flush()?;
- Ok(self.writer)
+
+ let mut this = ManuallyDrop::new(self);
+
+ // Extract every member that is not Copy and therefore should be dropped
+ let _resolved_schema = std::mem::take(&mut this.resolved_schema);
+ let _buffer = std::mem::take(&mut this.buffer);
+ let _user_metadata = std::mem::take(&mut this.user_metadata);
Review Comment:
`this.resolved_schema` is taken and dropped (on line 361) and `this.schema` is a reference and therefore does not need to be dropped
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #199:
URL: https://github.com/apache/avro-rs/pull/199#discussion_r2115759159
##########
avro/src/writer.rs:
##########
@@ -348,7 +354,18 @@ impl<'a, W: Write> Writer<'a, W> {
pub fn into_inner(mut self) -> AvroResult<W> {
self.maybe_write_header()?;
self.flush()?;
- Ok(self.writer)
+
+ let mut this = ManuallyDrop::new(self);
+
+ // Extract every member that is not Copy and therefore should be dropped
+ let _resolved_schema = std::mem::take(&mut this.resolved_schema);
+ let _buffer = std::mem::take(&mut this.buffer);
+ let _user_metadata = std::mem::take(&mut this.user_metadata);
Review Comment:
What about `this.schema` and `this.resolved_schema` ?
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: flush Writer on drop [avro-rs]
Posted by "Kriskras99 (via GitHub)" <gi...@apache.org>.
Kriskras99 commented on PR #199:
URL: https://github.com/apache/avro-rs/pull/199#issuecomment-2924490779
Thanks for taking the time to do an in-depth review!
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org