Refactor imap_flow::stream

[jakoschiko]

Previously there were two public types in imap_flow::stream:

• The trait Stream: This trait existed only for technical reasons. The user might think that they should implement it, which is wrong.
• The struct AnyStream. Handles the boxing, the pinning and the dynamic dispatch. This is the type that the user should interact with.

The new API looks like this:

• imap_flow::stream has now only a single type, the struct Stream
  • It can be constructed with _::new
  • It provides access to the underlying AsyncRead via _::read and _::read_mut
  • It provides access to the underlying AsyncWrite via _:::write and _::write_mut
• ClientFlow and ServerFlow provide access to the underlying Stream via _::stream and _::stream_mut

Closes #18

[coveralls]

Pull Request Test Coverage Report for Build 6539697912

• 29 of 64 (45.31%) changed or added relevant lines in 5 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.3%) to 41.416%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/receive.rs	3	5	60.0%
src/client.rs	1	7	14.29%
src/server.rs	1	7	14.29%
src/send.rs	5	14	35.71%
src/stream.rs	19	31	61.29%

Totals
Change from base Build 6513227947:	-0.3%
Covered Lines:	193
Relevant Lines:	466

---

💛 - Coveralls

[jakoschiko]

I'm not sure if this is really the best solution. We should discuss this.

[duesee]

Thanks!

Recapping to test my understanding:

The main improvement is that instead of pub trait Stream, we now have trait AsyncReadWrite (which doesn't need to be exposed.) This is possible by introducing the read, read_mut, ... methods that don't expose a trait pairing AsyncRead + AsyncWrite but gives separate handles AsyncRead, AsyncWrite.

Technically, we could also "inline" the AsyncReadWrite trait and impl all methods directly on Stream?

Are there any downsides to this solution despite it having more code?

Bonus:

• Better names AnyStream -> Stream ✅
• Better access .0 -> write_mut (see question) ✅

From my understanding, this PR is definitely an improvement. As we are still in an experimenting phase, I would propose to merge it after resolving the rename suggesstion. Then, I will rebase my work on the examples + tui and we can see how it goes.

[duesee]

Question: Should we rename these to reader, reader_mut, writer, writer_mut, instead?

[jakoschiko]

Technically, we could also "inline" the AsyncReadWrite trait and impl all methods directly on Stream?

This is not possible because dyn can only be combined with a single (normal) trait (yet).

Similarly dyn downcasting is not (yet) possible, so we need these methods on the trait.

Another option would be to implement AsyncRead and AsyncWrite for Stream. In that cause we could get rid of these read, ... methods. I already tried it and it was annoying because I needed to write a lot of delegating code. But it made the API even smaller, so we could discuss it.

[duesee]

This is not possible because dyn can only be combined with a single (normal) trait (yet).

Similarly dyn downcasting is not (yet) possible, so we need these methods on the trait.

Ah, sorry. I now see why it's not possible.

We could also get rid of Stream and dynamic dispatch altogether. Start with ClientFlow<S> and propagate the AsyncRead + AsyncWrite + Send + Unpin requirements. I just followed the compiler errors w/o thinking too much :-)

It's a bit... unwieldy, see duesee_jakoschiko/better-stream branch.

PS: Urgs, STARTTLS may require dyn :-/ Or IMAP's COMPRESS extension.

[jakoschiko]

We could also get rid of Stream and dynamic dispatch altogether. Start with ClientFlow<S> and propagate the AsyncRead + AsyncWrite + Send + Unpin requirements. I just followed the compiler errors w/o thinking too much :-)

This type parameter is parasitic and will effect many types. It will result in very, VERY ugly type annotations... e.g.

Proxy<ConnectedState<TcpStream, TlsStream<TcpStream>>>

But hey, that's how Rust works, it's worth a try.

PS: Urgs, STARTTLS may require dyn :-/ Or IMAP's COMPRESS extension.

Can you elaborate this more?

[jakoschiko]

... and propagate the AsyncRead + AsyncWrite + Send + Unpin requirements ...

Are you sure about Unpin? I think it's not necessary if we do Box::pin(stream) inside the constructor of ClientFlow and ServerFlow.

[duesee]

Are you sure about Unpin?

No. It was just a draft to see how bad it would be :-)

Can you elaborate this more?

When STARTTLS is used, we need to replace TcpStream with TlsStream w/o closing the connection. This is easy with Box<dyn Stream> but requires extra work w/o dynamic dispatch.

When COMPRESS is used, we need to replace TcpStream with DeflateStream w/o closing the connection.

Combined... we need to handle (and make room for) ...

• TcpStream,
• DeflateStream(TcpStream),
• TlsStream(TcpStream),
• TlsStream(DeflateStream(TcpStream))

... in our client.

[jakoschiko]

Interesting... I'm not sure how we should proceed. Maybe we should keep dyn trait. Maybe we should avoid both dyn trait and the type parameter and instead be more strict so we can prevent cases like DeflateStream<DeflateStream<TcpStream>>. I suggest to postpone this issue and continue with the other features first. Future knowledge might improve our decisions on this topic.

[duesee]

Okay! I feel that an enum with four states could be a bit annoying. But it would definitely be the tightest model of a sane IMAP reality.

Feel free to close this PR. Looking forward to revisit!

[duesee]

This gets out-of-sync. Shall we close for now? Maybe there are fragments we could still merge?

[jakoschiko]

I would like to keep this PR for irrational reasons.

[duesee]

Release it. It wants to be free!