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

Stream wrappers standardization #1568

Closed
wants to merge 3 commits into from
Closed

Conversation

bozaro
Copy link

@bozaro bozaro commented Apr 1, 2016

I wrote simple binding for lz4 compression (https://github.com/bozaro/lz4-rs) and got issue bozaro/lz4-rs#9.

The main problem is: there are not best practices for writing compression (encryption and etc) libraries.
Compression libraries need some method for "finish" work: write end stream mark and flush data.
This work should not run in drop() method: I prefer unexpected end of stream instead of success on reading incomplete stream.

As result I create this PR :)

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Apr 1, 2016
@nagisa
Copy link
Member

nagisa commented Apr 2, 2016

I do not think having these traits in some crate as opposed to the standard library precludes their de facto standardisation. If the crate turns out to be somewhat popular, it could be moved into nursery.

In my eyes to include something like that into the standard library you’d need some use-case in said standard library.

@bozaro
Copy link
Author

bozaro commented Apr 2, 2016

If the crate turns out to be somewhat popular, it could be moved into nursery.

I'm sure, that nobody will use crate with two traits. Its just no one will find it.

In my eyes to include something like that into the standard library you’d need some use-case in said standard library.

I know only one struct from standard library for which this trait useful:

std::io::BufWriter {
    fn get_ref(&self) -> &W;
    fn into_inner(self) -> Result<W, IntoInnerError<BufWriter<W>>>;
}

It differ with WriteWrapper by:

  • have another method names;
  • IntoInnerError allow unwrap output stream even on flush error (but this stream has undefined content).

I don't known standard structs for ReadWrapper trait.

@emberian
Copy link
Member

emberian commented Apr 5, 2016

I'm sure, that nobody will use crate with two traits. Its just no one will find it.

Not if ~every compression library depends on it and implements them. There is a community perspective here that you need to have. Further efforts like stdx lower the barrier even further. The Rust Way ™️ is clear here: this belongs in a separate crate. The standard for something added to std needs to be very high, and this doesn't cut it.

Ignoring that I don't think this is a good idea:

  • You need to provide more evidence that the proposed traits are actually what are desired, sufficient and useful for ~all cases.
  • The alternatives section does not actually explore other alternatives in this design space. There are obviously alternatives: each stream compression type defines its own right now.
  • These traits need better names.
  • The RFC text needs some editing for grammar, spelling, and tone.
  • The RFC characterizes this as "compression, encryption and etc" but doesn't discuss these other usecase.
  • This trait is more generic, I think: it is for streaming information into a container of any sort.
  • I'm not convinced that it's always the case that "finishing" the stream should return the underlying Read/Write/
  • I'm not convinced finish never wants an argument.
  • The statement that returning &R for reader (resp. writer) doesn't allow for modifying the state of the underlying object is false because of interior mutability. There's no reason not to have corresponding &mut versions.
  • finish needs to be able to return more information, for example statistics reporting that might be expensive to re-compute after the finish, or might be impossible to recover without keeping bookeeping information (for example, writing to a socket). If you don't, any information collected can't take into account the actions of the future finish.

@bozaro
Copy link
Author

bozaro commented Apr 5, 2016

You need to provide more evidence that the proposed traits are actually what are desired, sufficient and useful for ~all cases.

It is difficult to argue :)

The alternatives section does not actually explore other alternatives in this design space. There are obviously alternatives: each stream compression type defines its own right now.

This RFC has appeared due to the fact that most do not think about it. In many respects, this problem is specific to rust: in other languages I can simply store unsafe Write reference in some variable :)

These traits need better names.

Maybe.

The RFC text needs some editing for grammar, spelling, and tone.

Sorry, English is not my native language. I known, that my English is very poor.

The RFC characterizes this as "compression, encryption and etc" but doesn't discuss these other use case.

Compression is original use case. But this trait is useful from any state full transformation.

Distinctive feature: explicit needed to complete the work.

This trait is more generic, I think: it is for streaming information into a container of any sort.

I could not think of another real use case.

I'm not convinced that it's always the case that "finishing" the stream should return the underlying Read/Write/

I see no reason why it is not necessary to return Read/Write.

I'm not convinced finish never wants an argument.

There is always an exotic usage scenario. At the moment, I can not think of.

The statement that returning &R for reader (resp. writer) doesn't allow for modifying the state of the underlying object is false because of interior mutability. There's no reason not to have corresponding &mut versions.

I'm sure, that returning mutable reference is unsafe. Immutable reference guarantee that anyone can broken stream, but useful for getting statistics from wrapped stream.

finish needs to be able to return more information, for example statistics reporting that might be expensive to re-compute after the finish, or might be impossible to recover without keeping bookeeping information (for example, writing to a socket). If you don't, any information collected can't take into account the actions of the future finish.

In a perfect world, you want to return as much information as possible. But I think that in most cases this should be enough. For example, in generic case wrapped stream is useless on finish error, because it contains undefined state.

In any case, it is always possible to implement a method different signature for specific case.

@hgrecco
Copy link

hgrecco commented Apr 7, 2016

While I do agree that the RFC needs more work, I think that making readers and writers from different projects easier to compose is a great idea.

@aturon aturon self-assigned this May 9, 2016
@alexcrichton
Copy link
Member

🔔 This RFC is now entering its week-long final comment period 🔔

The libs team discussed this RFC during triage yesterday and the conclusion was that this doesn't seem quite ripe for inclusion in the standard library yet (as-is). We've been toying with the idea of explicit close methods and debating what the signature of that method looks like, and we'd be quite amenable to an RFC along those lines! For specifically stream wrappers, however, we thought it may be a somewhat roundabout method to solve that problem.

@alexcrichton alexcrichton added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label May 24, 2016
@bozaro
Copy link
Author

bozaro commented May 24, 2016

Unfortunately, I can not bring this RFC to the required quality level because of problems with the English language :(

@ruuda
Copy link

ruuda commented Jun 2, 2016

I ran into this scenario myself once. There is one issue with the current proposal: finish() taking self by value prevents it from being used as a trait object. This is important in cases where the writer is determined at runtime. For instance, you might decide to wrap the writer in a compressing writer based on a command-line flag.

Here’s an example:

trait Consume {
    fn consume(self);
}

let trait_obj: Box<Consume> = ...
trait_obj.consume(); // E0161

And the best workaround that I am aware of:

trait ConsumeBox: Consume {
    fn consume_box(self: Box< Self >);
}

impl <T: Consume > ConsumeBox for T {
    fn consume_box(self: Box<T>) {
        self.consume();
    }
}

let trait_obj: Box<ConsumeBox > = ...
trait_obj.consume_box(); // Fine

(Aside: I talked about this finish/finalize scenario at Rust Amsterdam. The slides are here, though they might be a bit cryptic without context.)

@cheme
Copy link

cheme commented Jun 3, 2016

I discover this rfc a bit late but I got this crate (not on cargo since I want to finish to implement some usecases before),where I also need an End method (see (ExtRead)[http://cheme.github.io/readwrite-comp/readwrite_comp/trait.ExtRead.html] and (ExtWrite)[http://cheme.github.io/readwrite-comp/readwrite_comp/trait.ExtRead.html] .

The main purpose of the crate is to compose reader such as compression or encryption (my use case is writing some tunnels). The reader reference has been put out of the trait (to compose extread trait implementation without having to care about the reader lifetime).

I drop the info here just in case it may interest anyone.

The interesting point is that I implemented Read and Write for structure including reader and extreader. For such composition, I use (CompW)[http://cheme.github.io/readwrite-comp/readwrite_comp/struct.CompW.html] as a standard Write trait, I had to use very dirty drop implementation to end the writing operation (flush got a different semantic), this is a major dropback because I need to silence write error or panic if something wrong happen.

In short, end method for writer and reader seems also really usefull to me (and I got direct use of them). +1

@eternaleye
Copy link

I'd like to note that streaming APIs of this type are often actively dangerous in cryptography, and should be avoided - they frequently open up many classes of attacks that should not even be on the table.

@cheme
Copy link

cheme commented Jun 7, 2016

Indeed, part of the 'never implement your own crypto' principle... (don't look at any tunnel.rs in some of my mydht crates). Composing crypto primitive being in itself the same.

Regarding the rfc (original usecase was compression), I remember the reason why I externalize the actual Reader/Writer from ReaderExt/WriterExt trait.

The point was that this way it was easier to design MultiW/MultiR which contains an array of the trait ReaderExt/WriteExt trait and use afterward a struct (CompW/CompR) to associate it with an actual reader.

@jan-hudec
Copy link

jan-hudec commented Jun 10, 2016

What problem would this proposal actually solve?

The Motivation section mentions lack of consistency between the existing wrappers, but then it does not go on to provide any example where the consistency would actually be needed for anything.

And I can't think about such example either: The only code that can reasonably call the .finish() method and unwrap the underlying stream is the code that creates the wrapper in the first place. Creating the wrapper needs parameters, that are different for each wrapper, so the construction can't reasonably be generic and therefore that code can't reasonably be generic over different wrapper types. Therefore that code does not gain anything from the .finish() method being provided by a trait.

@nagisa, if the use-case exists, anywhere, then the std::io::BufReader and std::io::BufWriter, being also stream wrappers, should implement the trait and that makes it clear the trait should go in std. At the moment I don't, however, see a point in creating a trait at all.

@alexcrichton
Copy link
Member

The libs team got a chance to discuss this RFC during triage yesterday and the conclusion was that like before we feel that the motivation of this RFC would be best served with a close method of some form. As a result the other conclusion was that we're going to close this for now, but an RFC about close would certainly be welcome!

Thanks regardless though for the RFC @bozaro!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants