async-std icon indicating copy to clipboard operation
async-std copied to clipboard

debug_asserts in read_line_internal are invoked on large lines over async-tls

Open rtyler opened this issue 4 years ago • 0 comments

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 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

rtyler avatar May 25 '20 18:05 rtyler