Refactor IrcClientFuture and ConnectionFuture to own the config

[theduke]

This refactors IrcClientFuture and ConnectionFuture to own the config instead of holding a reference.

This is unfortunately a breaking change. I didn't see a way to keep the old public API without the reference.

This is required though for dynamically connecting to additional servers on the same reactor.

The examples/multiserver.rs example only works because the Config structs are in the same scope that Reactor::run() is called, which is not possible in a dynamic (future) context, where you need to use Handle::spawn to initiate a new client.

[aatxe]

Can you elaborate more with an actual example program that this enables?

[theduke]

Sure, see below.

I'm using a separate thread here for simplicity, but unless I'm missing something this would be needed in any context where you want to dynamically create a new client on a shared Reactor.

I stumbled over this while implementing my current use case which didn't work due to the config not being owned: I'm writing a IRC log bot that collects logs and and stores them into files. The bot will have an API endpoint to add new servers (and channels), so it needs to connect to new servers on demand based on a API request, which would be inside a future itself.

let (shutdown_tx, shutdown_rx) = futures::sync::oneshot::channel::<()>();
let core = Core::new().unwrap();

let remote = core.remote();

std::thread::spawn(move || {
  let f = IrcClient::new_future(handle.clone(), config)
    .unwrap()
    .and_then(move |client::PackedIrcClient(client, client_future)| {
        client.identify().unwrap();
        let f = client
             .stream()
             .for_each(move |msg| {
                 debug!("message {:#?}", msg);
                 futures::future::ok(())
             })
             .join(client_future)
             .then(|re| {
                 futures::future::ok::<(), ()>(())
             });
         remote.spawn(f);
    });
}).unwrap();

core.run(shutdown_rx.and_then(|_| {
    futures::future::ok(())
})

[aatxe]

Ah, I understand now. Thanks! Can you look at the 0.14 branch and see if it's possible after the switch to tokio? If not, since this is a breaking change, it'll still need to based off that branch.

[theduke]

Unfortunately no, the problem with the lifetime is still the same.

If you are OK with the change I can rebase on top of 0.14.

I could also keep the API impact minimal by still taking a &Config as argument and cloning internally, but I think it would make more sense to just take a owned value everywhere, what do you think?

[aatxe]

I'd rather just be upfront about the cost of the clone, I think. Though, it probably happens infrequently enough in most cases to not make a huge difference. I think at some point I expressed a desire to have Configs be copy-on-write, but that change might be a pain and not necessarily helpful here.

[theduke]

Yeah the config never needs to be clone during processing, just at the setup, so the cost is really miniscule.

I'll rebase.

[theduke]

@aatxe rebased.

I also added a tiny second commit that replaces a deprecated re-export of ClientFuture.

I noticed a separate issue with the 0.14 branch but I'll create an issue for that (edit: https://github.com/aatxe/irc/issues/156).

[aatxe]

Thanks, looks good to me! 🍻