Seperate I/O from Telnet Parsing & Formatting

[JamesStewy]

Similar to #14, I would like to use this package with tokio to read and write to a telnet stream asynchronously.

I was going to offer a tokio implementation for this library, but realised that doing so, while also providing the existing or similar synchronous implementation, would require either extensive reworking or a parallel implementation due to the Read & Write traits of each system being incompatible. That is on top of the fact that in rust there are a large number of ways to read and write data such as blocking, non-blocking, futures, tokio, mio, etc. Telnet is an application layer protocol so the implementation of the underlying stream shouldn't matter.

So my suggestion is to provide an API in this package that simply parses incoming telenet data into events, and formats outgoing data, by passing buffers rather than reading or writing to the underlying stream. That way the application can use its own method for reading and writing.

From there we could:

1. replace the existing API in this package with the suggested API. This would allow for the removal of most if not all I/O from this package.
2. reimplement a similar API to the existing one using the suggested API as its base (to reduce redundant code and the number of tests). This would provide a low level and high level API to different applications.
3. create a new synchronous API more in line with Allow splitting a Telnet Connection into a Reading and a Writing half #7 (splits reads and writes) which also uses the suggested API as its base. This would allow for full-duplex connections.
4. do something else. I am open to suggestions.

I have made a rough implementation of the suggested API as well as a synchronous reader and writer here to get an idea of what I am proposing.

Is this something you would be interested in pursuing?

[SLMT]

Great suggestion! I agree that it would be better to separate I/O out of the implementation. I will take a look at your code in a few days, and then we can discuss how we proceed.

[SLMT]

Hi @JamesStewy, sorry for my late reply. Last time I was in a long vacation (Chinese New Year), so I was not able to process this in time.

I read though your code, and I think it is quite promising. This change definitely makes this lib more flexible to use. The only thing we may need to do is to add some test cases and docs for it.

Maybe we can start from opening a pull request and iteratively adding other stuffs? How do you think?

[JamesStewy]

No worries, I hope you enjoyed your break.

Currently there are quite a few options for the functionality that the final lib could provide. I was thinking if we can get an initial direction on some of these I can then rewrite what I have done in a clearer way (separating the new code into files, removing redundant existing code, etc.). I can then submit that as the PR and we can refine it from there.

Below is a list of a few of the key decisions I can think of:

1. In my original post I listed a few protentional options for how we could provide a synchronous API (or none at all). Do you have any thoughts on these? Maybe we could just start with no synchronous API (just the parser and formatter) then add from there.
2. I haven't put any thought into stream compression for my initial implementation. If we are to provide a synchronous API, this will need to be considered.
3. I have been playing around with providing std::io::Write and tokio::io::AsyncWrite trait implementations. However these traits require their implementations to write to the underlying stream at most once per call [1]. This combined with the need to escape IAC bytes makes a generic implementation tricky. What I was able to come up with here requires the user wrap the "write" call they are using inside a closure. It's not super clear but maybe that would be helped with some documentation. Do you have any thoughts on this?

I will also write docs and tests but I was thinking of leaving that until we have a better idea on what the final lib looks like.

[SLMT]

Hi, thank you for your understanding.

Here are my thoughts:

1. I think the best option for users is to provide both high level synchronous APIs with split reads and writes and low level APIs for parsing telnet commands like what you said in the original post. This should be the most flexible option for users. Of course, we can start from just the parser and the formater. Once everything works fine with test cases, we can then start implementing high level APIs.
2. I think we can leave the compression streams alone for now. After the main components work, we can then go back to check how to integrate the compression streams in the new APIs.
3. I am thinking maybe it means that we should not implement Write for the telnet writer. Since the telnet protocol is an application layer protocol, it is inevitable to add some other bytes when writing data. I check a few other libraries such as hyper. They also do not implement Write for their high level write APIs. I believe it is because they are treating the user data as 'objects' instead of 'bytes' so they do not need to implement Write to handle 'bytes' for users. However, if you thought it would be better to implement Write for the telnet writer, we could first check if there is any other library encountering the same issue like us.

If you are ok with these, we can start from making a check list and open a branch for maintaining the new implementation.