Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Give back Writer on Encoder::finish() #8

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

algesten
Copy link
Contributor

If using a Cursor<Vec<u8>> to write to memory, or an auto-removing temp file (https://docs.rs/tempfile/3.5.0/tempfile/fn.tempfile.html#resource-leaking), there needs to be some way of getting the captured writer back.

I suggest adding the writer as output of the finish call, so we can get ownership of it back.

@algesten algesten force-pushed the fix/writer-back branch 3 times, most recently from 5167e8e to 5355be8 Compare April 13, 2023 14:28
@AlexTMjugador
Copy link
Member

AlexTMjugador commented Apr 13, 2023

Thank you for using this crate and submitting a PR! ❤️

Since finish does not currently return anything interesting on success, I think that it is a sensible idea to yield the sink back there.

However, the standard library provides a by_ref method for every Write, the purpose of which is to avoid transferring ownership of a writer to methods and data types that take a Write implementation, such as VorbisEncoder, by mutably borrowing it instead.

Does your PR solve any problem that by_ref does not? (Just asking, it could be merged even if it didn't 😄)

@algesten
Copy link
Contributor Author

@AlexTMjugador thanks for the prompt reply!

My problem with using a borrowed writer is that the borrow lifetime parameter must leak upward into any wrapping struct.

In my case I have a struct Output which holds both the vorbis encoder as well as a mixer since they are used together.

struct Output {
  encoder: Encoder<File>,
  mixer: Mixer,
}

If this was Encoder<&'a mut File> it would force me to also have Output<'a>. It's possible, but "taints" my abstractions and breaks my encapsulation.

Copy link
Member

@AlexTMjugador AlexTMjugador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, that lifetime parameter propagation avoidance use case convinced me! I'd like to merge this after a little change 😉

Copy link
Member

@AlexTMjugador AlexTMjugador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind the previous review, other VorbisEncoder fields have destructors that must be run when tearing down an encoder, and unwrapping the sink around is easier to reason about for correctness.

Merging this! 🎉

@AlexTMjugador AlexTMjugador merged commit 0dee69c into ComunidadAylas:master Apr 14, 2023
@algesten
Copy link
Contributor Author

Thanks!

@algesten algesten deleted the fix/writer-back branch April 14, 2023 11:23
@AlexTMjugador
Copy link
Member

AlexTMjugador commented Apr 14, 2023

This change has just been published in vorbis_rs v0.3.0! 🎉

I also took the opportunity to do some minor tidy-ups to the codebase and yanked v0.1.0, as it contains a known soundness issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants