Add top-level function for starting a server with a provided Handle

[kierdavis]

I would like to run two tokio-based servers in parallel in my application, one of which is a Gotham HTTP server. As far as my understanding of tokio goes, it ought to be possible to create one event loop and register both services onto it.

Is there a way to start a Gotham server using a provided tokio Handle, rather than letting it create its own event loop? If not, and you feel that its a reasonable feature to implement, I'm happy to work on adding this behaviour.

[ChristophWurst]

Yes, that's possible. See #66 (comment)

[kierdavis]

Great, thanks! I'll check that out.

[bradleybeddoes]

Hi @kierdavis you absolutely can and @ChristophWurst has pointed out one option for doing that which should work for 0.1.x.

However we've recently merged in #89 which may have broken this method if you're using Gotham master (which represents the 0.2 release, I'd actually recommend using this if you're embarking on a new application, there are a lot of bug fixes and new features coming in 0.2)

What would be really useful, if you have the time, is a start_with_handler function that is essentially the same as https://github.com/gotham-rs/gotham/blob/master/gotham/src/lib.rs#L61 but does not create a Tokio core when you into the actual serve function, as it does now https://github.com/gotham-rs/gotham/blob/master/gotham/src/os/unix.rs#L46

[kierdavis]

Thanks for the info! I'm currently running v0.1 as that the latest available on crates.io, but I'm all for switching to 0.2 when it becomes available.

I'm happy to have a crack at implementing start_with_handler as you say. Looking at the sources it seems it should be trivial to pass a Handle down to where it's needed. One question I have regarding this is how to handle multithreading in this scenario - currently we create one event loop per thread, so would we want to accept a list of Handles or share one between all threads?

As my original question has been answered (@ChristophWurst's suggestion is working perfectly for me) I'll rename this issue to be more inline with the feature request side of things.

[bradleybeddoes]

So @smangelsdorf and I have just thrown this very discussion around ourselves.

What start does nicely now is ensuring that there is a Tokio Core per thread and we create n == CPU core threads at startup.

Our thinking is that having serve https://github.com/gotham-rs/gotham/blob/master/gotham/src/os/unix.rs#L41 and https://github.com/gotham-rs/gotham/blob/master/gotham/src/os/windows.rs#L103 change to be public and have both of them accept a Handle might be a smarter approach. The code we have right now to create a Tokio Core and obtain a Handle can be moved up into the respective loops.

That would mean it would be up to the implementing application to create a core per thread (assuming they wanted that) to mirror what Gotham would normally do in start/start_with_num_threads. Having it as serve rather than start will hopefully re-enforce the difference.

Thoughts on this approach?

[kierdavis]

I like this approach. My only concern is that serve also requires a TcpListener, so I propose making tcp_listener public too.

[kierdavis]

I suppose listen and SocketQueue in os/windows.rs would also need to be public?

[bradleybeddoes]

That seems reasonable for tcp_listener @kierdavis. Thoughts on also moving tcp_listener into the os module?.

From the brief look at the Windows side I've just had it does seem like SocketQueue would need to enter the public API but I am not convinced that listen would need to be made pub, perhaps I am missing a nuance there.

cc @smangelsdorf.

[smangelsdorf]

Thoughts on also moving tcp_listener into the os module?

Maybe under os::common? Keeps it out of Gotham's top-level, which I presume is the intention.

it does seem like SocketQueue would need to enter the public API

Under the current implementation, yes, that's my belief too.

I am not convinced that listen would need to be made pub

Not in its current form. listen currently creates its own Core, which goes against the point of this ticket anyway. Whether a future incarnation of listen should be exposed is hard to say without seeing the result of implementing this feature.

It's currently platform-specific too, so it would be a difference in the per-platform API if it was pub. The fewer per-platform deviations there are, the easier it'll be for applications to use this.

[kierdavis]

Whether a future incarnation of listen should be exposed is hard to say without seeing the result of implementing this feature.

@smangelsdorf see #121, in which the first half of this feature (decoupling core creation from serve and listen) has been implemented.

It's currently platform-specific too, so it would be a difference in the per-platform API if it was pub.

That's a good point. Perhaps this could be encapsulated in a platform-independent way as a public function that returns a set of top-level futures (i.e. the object returned by listen and serve) for the user to distribute over event loops however they wish? With this approach there's no longer a need to make listen, serve, SocketQueue and everything else public anymore.

This approach does however leave the problem that the construction of each future requires a Handle derived from the event loop it will eventually be run on - although some closure wizardry could probably solve this.

At this point I'd like to mention that now I've dug deeper into gotham's sources and learnt more about how both gotham and tokio work, my application would probably be better off if I just ran my other service in its own event loop in another thread, analogous to how gotham spawns instances of serve/listen (it's not as if Rust makes safe multithreading difficult, anyway :P). I'm more than happy to continue working on this feature though, if you believe there are still valid use cases for it.

[smangelsdorf]

Brain dump:

I think #121 has taken this in the right direction, though as I was alluding to earlier, I'd want us to prevent the divergence between the gotham::os::unix and gotham::os::windows APIs. Since they get exposed in the same way, a difference between the two means that app code would have to add conditional compilation or else be able to compile on only one platform.

Obviously I'd like us to avoid as much standard boilerplate as possible for a correctly implemented Gotham app, but this especially applies to conditional compilation stuff.

One concept I have in mind (which is thoroughly untested) for going forward with this is to change the serve function to be something like:

fn serve<'a, G, NH>(
    listener: G,
    protocol: &'a Http,
    new_handler: Arc<NH>,
    handle: &'a Handle,
) -> Box<Future<Item = (), Error = io::Error> + 'a>
where
    G: GothamListener,
    NH: NewHandler + 'static,
{
    // ...
}

trait GothamListener {
    // Something which basically acts as a `Stream<Item = (TcpSocket, SocketAddr)>`. Also possible
    // that we could just use the `Stream` trait directly instead of introducing our own, if we
    // found a way to do it within the orphan rules.
}

With that, we could add a new gotham_listener function that replaces the current tcp_listener, and in an OS-specific way:

// os/unix.rs
/// Creates a server socket which can be cloned for `accept()` to be called in 
/// each Tokio `Core`.
fn gotham_listener<A>(addr: A) -> (TcpListener, SocketAddr)
where
    A: ToSocketAddrs,
{
    // ...
}

impl GothamListener for (TcpListener, SocketAddr) {
    // ...
}

// os/windows.rs
/// Spins up a thread in the background which holds the sender side of the
/// `SocketQueue` which is returned here.
fn gotham_listener<A>(addr: A) -> SocketQueue
where
    A: ToSocketAddrs,
{
    // ...
}

impl GothamListener for SocketQueue {
    // ...
}

Errors and omissions aside, this should allow the same app code to compile and run on all supported platforms.

Thoughts, improvements, etc. welcome of course 😁

[whitfin]

#254 should resolve this, although it will ship with v0.3.0 which uses tokio rather than tokio_core. As such the new function accepts a TaskExecutor for a custom Runtime, rather than a Handle.