add MultiReader and TeeReader #62
Conversation
chapter3/src/3_7/tee_reader.rs
Outdated
| @@ -0,0 +1,50 @@ | |||
| use std::io::{Read, Write}; | |||
|
|
|||
| struct TeeReader<R, W> | |||
There was a problem hiding this comment.
複数の問題で使うかはわかりませんが、TeeReaderも単機能で汎用性が高いので lib に移しませんか？🙂
lib/src/io/multi_reader.rs
Outdated
| /// Ok(()) | ||
| /// } | ||
| /// ``` | ||
| pub struct MultiReader<R> { |
There was a problem hiding this comment.
<R: Read> に制約して良いと思います。
（Read以外はコンストラクタがないので実際は作れませんが）
lib/src/io/multi_reader.rs
Outdated
| } | ||
| None => return Ok(0), | ||
| } | ||
| self.pos += 1; |
There was a problem hiding this comment.
ここは「n個目のReadから読み切ったらn+1個目のReadに乗り換える」という動作をしていると思いますが、
Vec<R> の代わりに VecDeque<R> を使えば self.readers.pop_front() で先頭のReadを捨てることができ、よりわかりやすく記述できると思います。
There was a problem hiding this comment.
あー、VecDeque 使ってみるのはありかもしれませんね。VecDeque なら for r in &mut self.readers みたいな形でイテレータ回すようにできるかも…？ちょっと検討してみます。
lib/src/io/multi_reader.rs
Outdated
| /// } | ||
| /// ``` | ||
| pub struct MultiReader<R: Read> { | ||
| readers: Vec<R>, |
There was a problem hiding this comment.
これだと同じ型のreadersしか持てないけど大丈夫でしょうか？
GoのMultiReaderがどうなってるのかはあまりわかってないですが。
https://golang.org/src/io/multi.go?s=1269:1311#L13
この定義的にはReaderなら何でも受け取れそう。
There was a problem hiding this comment.
Box<dyn Read> のほうが正しそうですね…！
| match self.current { | ||
| Some(ref mut r) => { | ||
| let n = r.read(buf)?; | ||
| if n > 0 { | ||
| return Ok(n); | ||
| } | ||
| } | ||
| None => return Ok(0), | ||
| } | ||
| self.current = self.readers.pop_front(); |
There was a problem hiding this comment.
Goの実装に依るのですが
ここの実装はreadersの切れ目で使う側の実装によっては強制的に読み込みが終了してしまう気がしています。
一般的にReadを使う場合でRead読み切ったかどうかを判断する場合したのようなコードを書きそうな気がしています。
let mut buf: [u8; 128];
let mut some_reader;
while some_reader.read(&mut buf) >= buf.len() {
...
}その場合にReadの切れ目でbuf.sizeより小さな数字が返ってくると中途半端な状態で終了してしまいそうかなと思いました。
There was a problem hiding this comment.
Go の実装はこちらですね（ただ、もともと参考にはしていなくて、だいぶ実装の形が違っています）
https://golang.org/src/io/multi.go?s=2446:2488
Go の実装に寄せるのはありかもです。
ここの実装はreadersの切れ目で使う側の実装によっては強制的に読み込みが終了してしまう気がしています。
申し訳ないですが、ちょっと具体的な例がイメージできておらず…！なにかコーナーケースをパッと思いついたりしますでしょうか？修正するにしても、理解を深めてから修正したいなと思っており。
There was a problem hiding this comment.
コーナーケースの具体例とその上での僕の想定は以下になります。(github上で書いたので雰囲気Rustですが)
#[test]
fn multi_reader_end_of_first_reader() {
let header = "---- HEADER ----\n".as_bytes();
let content = "Example of MultiReader\n".as_bytes();
let mut multi_reader = MultiReader::new(vec![Box::new(header), Box::new(content)]);
let mut buf: Vec<u8> = vec![0; 256];
let size = multi_reader.read(&mut buf).unwrap();
assert_eq!(size, header.len() + content.len()); // ここが現状の実装だとsize == header.len()になってそう？
}There was a problem hiding this comment.
ちなみに、Goの実装を読んでみたら僕の読解が正しければ errとしてEOFは返さないけどbuf.len未満のsizeを返す(size == header.len()) になる感じでした。
Rustは通常のReadではEOFをErrとして扱わないので厄介ですね。
There was a problem hiding this comment.
上記コードを実行してみた結果、たしかに落ちています。考えてみますね。
running 1 test
test io::multi_reader::multi_reader_end_of_first_reader ... FAILED
failures:
---- io::multi_reader::multi_reader_end_of_first_reader stdout ----
thread 'io::multi_reader::multi_reader_end_of_first_reader' panicked at 'assertion failed: `(left == right)`
left: `17`,
right: `40`', lib/src/io/multi_reader.rs:67:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
io::multi_reader::multi_reader_end_of_first_reader
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
There was a problem hiding this comment.
@yuk1ty
めちゃくちゃ気になっちゃっていろいろ調べてみました。
結論現状の実装のままで問題なさそうです。
Rustの標準ライブラリのコード(read_to_endとかChainとか)を読んでみたんですがRustのReadでは返り値0以外は特別扱いしてはいけないみたいです。
ですので、僕の書いた
let mut buf: [u8; 128];
let mut some_reader;
while some_reader.read(&mut buf) >= buf.len() {
...
}上のようなコードは良くないみたいです。(コンパイル時に検知できないのはRustらしくないなとは思いましたが…)
There was a problem hiding this comment.
あと、Readを改めて調べてみたら Read::chainっていうのを見つけたんですがこれを使うとMultiReaderを簡単に実装できないですかね？(学習用なら残しても良さそうですが)
There was a problem hiding this comment.
これを使うとMultiReaderを簡単に実装できないですかね？(学習用なら残しても良さそうですが)
実装してみたところ、出来たけどあんまり簡単じゃなかったです。
There was a problem hiding this comment.
ありがとうございます。
Read::Chain は2つずつしかつなげられなくて、結構工夫がいりそうだったんですよね。car-cdr みたいな感じでリストを組んで、再帰的にたどる手でいけるかなとか妄想はしましたが、Vec をはじめとするベクタ系の方が実装が直感的と思って、そちらを採用していたという経緯があります😌
There was a problem hiding this comment.
car-cdr みたいな感じでリストを組んで、再帰的にたどる手でいけるかなとか妄想はしましたが、Vec をはじめとするベクタ系の方が実装が直感的と思って、そちらを採用していたという経緯があります😌
おっしゃるとおりですね。
参考までに実装を書いておきます。
pub struct AnotherMultiReader {
reader: Option<Chain<Box<dyn Read>, Box<dyn Read>>>,
}
impl Read for AnotherMultiReader {
fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
self.reader.as_mut().map(|x| x.read(buf)).unwrap_or(Ok(0))
}
}
impl AnotherMultiReader {
pub fn new(readers: Vec<Box<dyn Read>>) -> Self {
Self::new_impl(readers.into())
}
fn new_impl(mut readers: VecDeque<Box<dyn Read>>) -> Self {
let first = readers.pop_front();
first
.map(|r| {
let remain: Box<dyn Read> = Box::new(AnotherMultiReader::new_impl(readers));
Self {
reader: Some(r.chain(remain)),
}
})
.unwrap_or(Self { reader: None })
}
}メリット(AnotherMultiReaderの)
- コーナーケース系のエンバグは少なさそう(Chainが十分テストされてるはずなので)
- stateを自分で管理していないので楽。
デメリット
- 実装が非直感的
- 教育的なコードではない(と感じる)
という感じなのでそのままで大丈夫だと思います。
|
@yuk1ty Draftですが |
|
コメントを付与したら review ready になるので、コメントを付与しちゃいますね |
| pub struct MultiReader { | ||
| readers: VecDeque<Box<dyn Read>>, | ||
| /// Points to current element while reading. | ||
| current: Option<Box<dyn Read>>, |
There was a problem hiding this comment.
直感的にはこれは常に readers.front() の結果になるので、このように別途フィールド（ステート）として持つ必要はないと思いますが、いかがでしょう？
対象の Issue
#15
動作確認結果
Go
Rust
MultiReader 側
TeeReader 側