debug_asserts in read_line_internal are invoked on large lines over async-tls
- Dominant language
- Rust
- Stars
- 4.1k
- Forks
- 339
- PR merge metrics
- No merged PRs in 30d
Description
I wasn't sure whether to file this in async-tls or in this repository, since the two crates are intertwined for this. When using `async-tls` and `BufReader`, lines larger than the default sizes (e.g. 8k). This can cause [these debug_assert! calls to fire](https://github.com/async-rs/async-std/blob/master/src/io/buf_read/lines.rs#L73) and likely mean something is really wrong underneath the covers.
I've created the following reproduction case, which doesn't have any issues with plaintext sockets, only with TLS sockets: https://gist.github.com/rtyler/a17fff6be72fc3482023b6be32e338eb
We discussed this briefly in `##rust` and there were some useful suggestions:
```
10:33:42 rtyler | yes I am aware, but it crashes all my test builds which isn't ideal either :)
10:34:14 danieldg | and it's likely handling the strings incorrectly in the release build
10:34:53 danieldg | if those asserts trigger, it'll move strings from one line to another
10:35:16 rtyler | I'm working on a small repro for filing the issue
10:35:18 Arnavion | The async-std implementation seems fine to me
10:35:37 Arnavion | What's the bug?
10:35:59 Arnavion | Bad things only happen if it's polled after it's yielded an Err() once, which is within spec
10:36:08 danieldg | without testing: an error on the second read during a line will return an error, but resuming reads afterwards will produce
| bad results
10:36:22 Arnavion | Exactly
10:36:47 danieldg | it never says you have to stop using it after it returns err
10:37:03 Arnavion | IIRC Stream impls are allowed to do that
10:37:29 Arnavion | There was a big discussion about it in the 0.1 days
10:37:45 danieldg | I would assume that only applied to Poll::Ready(None)
10:37:53 danieldg | not Poll::Ready(Some(Err(_)))
10:39:01 Arnavion | https://github.com/rust-lang/futures-rs/issues/206 Ah, I misremembered. The change was rejected
10:39:33 danieldg | so the bug might be that it should start returning None after it returns an error once
10:39:33 Arnavion | So yeah, they need to maintain state so that they return Ready(None) after a previous Ready(Some(Err))
10:39:49 danieldg | or, fix for real and just recover properly
10:40:13 danieldg | there's no reason that I can see for it to hold both a String and a Vec
10:40:33 danieldg | the Vec alone can be built, replaced, and .into'd to a String to return
10:40:37 Arnavion | That should be the caller's decision. And given it's trying to behave like libstd's lines() it shouldn't recover for that
| reason either
10:41:10 danieldg | returning None is the right solution then
10:41:13 Arnavion | Yes
```
Contributor guide
Assessment
This issue has not been assessed yet.