osa1 / tiny

A terminal IRC client
MIT License
1.01k stars 60 forks source link

Adding other chat backends? #47

Open saethlin opened 6 years ago

saethlin commented 6 years ago

If I wanted to make a fork of tiny that can speak to other IRC-like chat services where would I start? I've poked around in the code and can't seem to find the part that reads from a connection and converts to an internal data structure.

osa1 commented 6 years ago

If you want your backend to work with tiny's current backend (e.g. in a single tiny process both IRC connections and your backend works) this'll require some refactoring in tiny.

I was hoping to do something like this myself so maybe we can work it out.

To summarize current design:

can't seem to find the part that reads from a connection and converts to an internal data structure

This happens in Conn.rs, read_ready() method. This method returns ConnEvs (fills the buffer passed by the caller), which are then handled by the caller to update TUI. See the handle_conn_ev() method.

It seems to me like we need to refactor the code a little bit to support different types of Conn.

Current Conn would become IrcConn. handle_conn_ev() would only handle IrcConn events.

Another Conn type (e.g. XMPPConn) would also have the same set of types and methods. E.g.

struct XMPPConn {}
enum XMPPConnEv {}
pub fn handle_xmpp_conn_ev(conn: &mut XMPPConn, ev: XMPPConnEv, tui: &mut TUI);

We could of course make Conn a trait, and event type an associated type.

Out of curiosity, what backend are you planning to implement?

In the meantime I think you can implement your client by using tiny's TUI as a library. The TUI could actually be made a library, it doesn't have any IRC-specific stuff, so it's somewhat reusable in other types if messaging apps. (although some of the functions you may need may not currently exists in tiny's TUI because it's implemented with tiny's use case in mind, but I think it's easy to extend with new UI interactions)

saethlin commented 6 years ago

I'm interested in bringing all the various messaging apps I use into one terminal interface, where I can seamlessly switch between them. I currently use Discord, Slack, Facebook Messenger, SMS via Pushbullet, and IRC throughout the day and I think it would be cool to have them all inside Tiny. (obviously there will be some loss of functionality)

Following your suggestion I've been implementing a Slack backend. I think I got read_ready working, and so far these are my thoughts:

I think we should have a Conn trait that requires only a few methods. It probably needs at least one constructor but I don't know what that looks like (the two you have now are IRC-specific but the seem in the right vein). I think read_ready is a good way to get data from the internet into tiny, but I think some work needs to be done moving data in the other direction. Maybe we should have a write_ready that takes a buffer of TinyEv? I would like to keep the ConnEv system, and I think it's sensible to let different backends produce only a subset of them, and only handle a subset of TinyEv. Maybe they return something to indicate that event isn't valid?

osa1 commented 6 years ago

I'll write more later but just wanted to point out

Maybe we should have a write_ready that takes a buffer of TinyEv?

We already have this, see Conn::write_ready (https://github.com/osa1/tiny/blob/master/src/conn.rs#L444-L455).

saethlin commented 6 years ago

Oh duh. I think I was just confused because it doesn't look like that method processes the events.

osa1 commented 6 years ago

I'm interested in bringing all the various messaging apps I use into one terminal interface, where I can seamlessly switch between them. I currently use Discord, Slack, Facebook Messenger, SMS via Pushbullet, and IRC throughout the day and I think it would be cool to have them all inside Tiny. (obviously there will be some loss of functionality)

This is great! I was thinking of doing the same for slack (which we use at work) and gitter (some OSS I use use it).

OK so I have this idea:

Instead of connections returning (or filling in) a vector of events, for TUI updates I think we could let connections update their TUI directly. This removes the need for connection-specific events.

Connections have three update functions:

These handle TUI updates themselves.

For updating tiny's state (rather than a connection's state) we implement TinyEv. TinyEv is what a connection returns to tiny's event loop.

Then for passing input from TUI to connections, we implement TUIEvent.

In summary it'd look like this:

use logger::Logger;
use tui::TUI;
use tui::tabbed::MsgSource;

/// Every `Conn` needs to be `Evented` to be useful
trait Conn {
    /// Called when `Conn`s socket is ready for reading.
    ///
    /// * `tui`: TUI handle that can be used to update tabs associated to this connection.
    /// * `evs`: Events for tiny to handle.
    /// * `logger`: Logger handle for this connection.
    ///
    fn read_ready(&mut self, tui: &mut TUIHandle, evs: &mut Vec<TinyEv>, logger: &mut Logger);

    /// Called when `Conn`s socket is ready for writing and the socket is registered for write
    /// events.
    ///
    /// Arguments have same meaning with `read_ready`'s arguments.
    ///
    fn write_ready(&mut self, tui: &mut TUIHandle, evs: &mut Vec<TinyEv>, logger: &mut Logger);

    /// Called every second.
    fn tick(&mut self, evs: &mut Vec<TinyEv>, logger: &mut Logger);

    /// Called when a command was sent to this connection.
    fn handle_cmd(&mut self, cmd: String, args: Vec<String>, evs: &mut Vec<TinyEv>, logger: &mut Logger);

    /// Called when an input was received in a tab that's associated with this connection.
    /// Implementations can return events to tiny using `evs` parameter.
    fn handle_tui_event(&mut self, ev: TUIEv, evs: &mut Vec<TinyEv>, logger: &mut Logger);
}

pub enum TinyEv {
    /// Drop this connection
    ConnectionClosed,
    // what else?
}

pub enum TUIEv {
    Input {
        msg: Vec<char>,
        from: MsgSource,
    }
}

/// A handle that allows updating tabs for a `Conn`.
pub struct TUIHandle {}

We then implement Conn for each backend, and extend TinyEv and TUIEv for as we need more functionality for backends.

Does that make sense? How about this plan: I create create a new branch and implement this trait and port current IRC backend to it. You can then try to implement another backend using this API and we can see what's working OK and what's not, update, and repeat.

saethlin commented 6 years ago

I like that you've shrunk the requirements for a connection (the existing Conn has a lot of public methods), but I don't understand the role of each of these methods. Here's how I see them:

read_ready will be called when tiny thinks there are messages to be read from me. I store them internally, then modify the TUI directly when I can and pass tiny an event when I need something more complicated.

write_ready Where's my input here? In read_ready these same arguments were outputs. Do I read from the vector of TinyEv, then add the sent message to the TUI?

tick lets a Conn do timeout handling. This may be stubbed out by non-IRC backends because they run on websockets that do their own timeout stuff.

handle_cmd this is how do my /join or whatever is appropriate for this kind of connection

handle_tui_event I'm not sure what I can do with this, don't I already know about all inputs received in tabs associated with me because that's my job?

The plan sounds good to me. I've been working on a Slack backend, and so far it looks not all that hard (I've written Slack stuff before). I think I've almost got something together for PushBullet, but their docs seem very incomplete so that could get interesting.

I can already tell you that the batched setup that you have for dealing with IRC connections does not play very well with the websocket interface used by nearly every other chat service. Event loops and handlers are the easiest thing to do, so right now I have a handler that just sits in its own thread and pushes into an Arc<Mutex<VecDeque<slack::Event>>> as events arrive. It might be worthwhile to mutate the TUI as events appear. Maybe the constructor of a Conn is passed an Arc<Mutex<TUIHandle>>?

saethlin commented 6 years ago

Feedback on the proposed Conn trait:

Some commands may produce an error. handle_cmd should either get a handle for the TUI or we should support error notifications via a TinyEv.

EDIT: Just noticed some commands want to know which channel we're in. More reason to have a handle to the TUI I think.

osa1 commented 6 years ago

As far as I understand you're thinking a futures/threads based API whereas I was thinking of a mio-based one.

In mio-style code you can't tell if a socket your backend is using have data to read or have space in its buffer to write. So that's why we have read_ready and write_ready. Those are called by the mio event loop and they notifies that the socket you're using is ready for reading or writing.

Instead if we design threads-based API then every backend can take care of their sockets. In a backend both futures and mio could be used.

If we design a futures-based API ... well first of all futures API is terrible IMHO. Secondly, because futures supposed to work with event loops but also with thread pools you have to design your API with threading in mind. So for example you need to pass stuff wrapped with Arc<Mutex<...>> rather than just passing &mut .... This is possible in mio-based design because clearly event loop calls read_ready, write_ready and other methods in a loop, there's no concurrency at all.

read_ready will be called when tiny thinks there are messages to be read from me. I store them internally, then modify the TUI directly when I can and pass tiny an event when I need something more complicated.

This is correct. tiny thinks there are messages to be read from you when mio thinks so as described above.

write_ready Where's my input here? In read_ready these same arguments were outputs. Do I read from the vector of TinyEv, then add the sent message to the TUI?

You get your inputs from TUI in handle_tui_event, and fill in your outgoing message buffers according to TUI inputs. However, this doesn't mean you can write the data directly to the socket (the socket may not have any space left), that's done in write_ready.

So

tick lets a Conn do timeout handling. This may be stubbed out by non-IRC backends because they run on websockets that do their own timeout stuff.

Hmm so they use threads and/or futures. OK.

handle_cmd this is how do my /join or whatever is appropriate for this kind of connection

Exactly.

handle_tui_event I'm not sure what I can do with this, don't I already know about all inputs received in tabs associated with me because that's my job?

This is where you get inputs from TUI. So suppose someone entered hello world in tab #foo. You get that information as a TUIEvent in this method. Depending on the backend this may mean sending a message to channel #foo, for example.

I can already tell you that the batched setup that you have for dealing with IRC connections does not play very well with the websocket interface used by nearly every other chat service

Right.. So maybe designing an API around a mio-based event loop is not a good idea (although I suspect it's possible to implement adapters from futures-based APIs to mio-based APIs and the other way around although I hvaen't tried this yet).

As mentioned above, nice thing about about the current design is you don't have to think about concurrency, you can share things as &mut ... instead of Arc<Mutex<...>>. But maybe this is something we can let go to have a more convenient API.

The options I see:

Some commands may produce an error. handle_cmd should either get a handle for the TUI or we should support error notifications via a TinyEv.

EDIT: Just noticed some commands want to know which channel we're in. More reason to have a handle to the TUI I think.

Ah, this makes sense. In fact current handle_cmd already has a handle to the TUI.

osa1 commented 6 years ago

Btw is your code open source yet? I'd love to take a peek to get a better understanding at what we need from the backend API.

saethlin commented 6 years ago

I'm totally down for doing this with mio. I'm just not sure how mio figures out that my connection can be read from or written to...

Code is here. I butchered my conn.rs quite a bit, so the code I've been working with doesn't get to the borrow checker when I cargo check it. Should probably fix that...

osa1 commented 6 years ago

I'm totally down for doing this with mio. I'm just not sure how mio figures out that my connection can be read from or written to...

mio just decides whether the socket your connection is using can be read of written to.

This is how event loops work, under the hood it uses epoll() system call (or poll(), select() etc.).

When you want to send data to the remote side of your connection, you tell the event loop to notify you when your socket is readay for writing, by calling your write_ready.

Let's see how tiny does this. Suppose we want to send a PRIVMSG. We call this Conn method:

pub fn privmsg(&mut self, target: &str, msg: &str) {
    self.status.get_stream_mut().map(|stream| {
        wire::privmsg(stream, target, msg).unwrap();
    });
}

Which uses stream::Stream's Write impl.:

impl<'poll> Write for Stream<'poll> {
    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
        match *self {
            Stream::Tcp(ref mut s) =>
                s.write(buf),
            Stream::Tls(ref mut s) =>
                s.write(buf),
        }
    }
}

Note that Stream has a reference to mio event loop to be able to register itself for write events (so that the event loop notifies it when its socket is ready for writing via the write_ready method).

Now a stream can be either TlsStream<TcpStream> or TcpStream. Let's skip a layer of indirection and look at TcpStreams Write impl which is used in this method:

impl<'poll> Write for TcpStream<'poll> {
    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
        // TODO inefficient when the socket is already ready for writing
        self.out_buf.extend(buf);
        reregister_for_rw(&self.poll, self.inner.as_raw_fd());
        Ok(buf.len())
    }
}

See how TcpStream also has a reference to the event loop. In this code we don't actually send any data, we just register our socket for write events and then write the data to our outgoing message buffer.

Then when the socket is ready for sending, the event loop calls TcpStream::write_ready() (actually it calls Conn::write_ready(), which in turn calls TcpStream::write_ready):

impl<'poll> TcpStream<'poll> {
    pub fn write_ready(&mut self) -> io::Result<()> {
        let to_send = self.out_buf.len();
        match self.inner.write(&self.out_buf) {
            Ok(bytes_sent) => {
                self.out_buf.drain(0..bytes_sent);
                let register = if bytes_sent == to_send {
                    reregister_for_r
                } else {
                    reregister_for_rw
                };
                register(&self.poll, self.inner.as_raw_fd());
                Ok(())
            }
            Err(err) => {
                reregister_for_rw(&self.poll, self.inner.as_raw_fd());
                Err(err)
            }
        }
    }
}

This is where we do the actual sending and update our outgoing message buffer.

Hopefully this clarifies mio-based API.

I'm not saying this good or convenient or anything like that. I'm just explaining the design. I suspect that this is not as convenient for other libraries (especially fuutres-based ones) so we may want to design something different.

osa1 commented 6 years ago

Hmm I think this is not going to work -- for this API to work you'd need read_ready and write_ready methods for the entire stack. So, if you're using slack, which probably uses reqwest under the hood, you need write_ready method for slack which will need write_ready for reqwest.

I think we should go with threads-based API and let the backends choose whatever method they like. Just fork a thread for each backend, pass a TuiHandle (which lets backends update their tabs no other tabs, so this is not shared and can be directly moved to a thread), and we're done.

Only problem is we need to figure how to poll TuiHandle for TUI events.

saethlin commented 6 years ago

I now own a chromebook so I really want these other backends in place.

I'm tempted to say that we should figure out an interface between connections and the rest of tiny that's decently clean and just run with it for now. If we have threads all over, I'm sure they can be replaced with the upcoming async features (we're on nightly already).

The biggest hurdle for me on getting something up and running is that all of tiny's internals are tightly locked to the way a Conn works. I'm not sure I'm up to the task of moving the current Conn into the new implementation. Is this something you're interested in doing? If not I could really do with some guidance.

It just occurred to me that I might be able to hack the new trait-based interface into tiny by adding a new member to the Tiny struct.

osa1 commented 6 years ago

I've been working on this and I started to think that we should implement a futures-based API and implement backends on top of that.

I think for most backends we need one of these three things:

So it seems to me that if we use futures we don't have to spawn threads per backend.

In this design we'd pass a Receiver<TuiEvent> and a tokio Handle to backends. Using Handle they can spawn async tasks, using the receiver they can read TUI events.

All backend methods would also get a Arc<Mutex<TuiHandle>> that allows them to modify their tabs or create new tabs.

I'll try to design the API in more details and try to port current IRC backend but there's one problem that I don't know how to solve; I don't know how to read stdin using tokio and futures.

Once I figure that out I should be able to try this design.

The biggest hurdle for me on getting something up and running is that all of tiny's internals are tightly locked to the way a Conn works. I'm not sure I'm up to the task of moving the current Conn into the new implementation. Is this something you're interested in doing?

Yes, and I can also provide guidance ... However I'm a bit busy these days so I can't give any ETAs..

osa1 commented 6 years ago

So I've been studying futures lately and I think I'm getting back to the thread-based API idea. futures ecosystem (tokio and co) are such a mess and it's impossible to do even the simplest things like reading stdin as &[u8]. (I think it should be possible, but I couldn't do it after hours of research)

So maybe something like this would work:

In addition, I want to keep current backend working without spawning any threads so another interface should be supported.. More on this later.

saethlin commented 6 years ago

I've been fiddling with futures and they look like a hot mess to me too. My compile errors see unintelligible and I can't find anyone on IRC or discord that can help. Fortunately, this now exists: https://crates.io/crates/may

osa1 commented 6 years ago

I was excited about MAY too, but unfortunately it seems like it's deeply broken: https://github.com/Xudong-Huang/may/issues/6 the problem is not specific to MAY, we just can't implement work-stealing threads in Rust because of issues with TLS.