with_handle NewService lifetime overly restrictive?

[alex]

https://github.com/tokio-rs/tokio-proto/blob/master/src/tcp_server.rs#L106-L115

Currently the NewService instance returned by F must be Send + Sync. Is this necessary? My read of the code is that a distinct NewService instance is created for each thread, and never moved from the thread. (https://github.com/tokio-rs/tokio-proto/blob/master/src/tcp_server.rs#L184-L186)

The result of this lifetime is that actually using the handle is difficult. For example, the following code doesn't work because the closed over handle is not Sync:

    let new_service = Arc::new(move |handle| {
        let http_client = new_http_client(&handle);
        Ok(HttpHandler {
            templates: templates.clone(),
            http_client: Arc::new(http_client),
            logs: logs.clone(),
            handle: handle,
        })
    });
    tcp_server.with_handle(move |handle| {
        let new_service = new_service.clone();
        move || new_service(handle.clone())
    });

My impression is that this type of use case is what was with_handle was intended for.

I was able to work around this with the completely insane:

    tcp_server.with_handle(move |handle| {
        // TODO: this doesn't seem right...
        let remote = handle.remote().clone();
        let new_service = new_service.clone();
        move || new_service(remote.handle().unwrap())
    });

which seems to confirm that everything is on the same thread.

At a minimum, better documentation of this pattern seems valuable.

[polachok]

I'm having the same issue and came to the same conclusion by looking at TcpServer code.

[matt-williams]

I hit this too and came to the same conclusion - removing Send + Sync bounds seems like the right fix to me (i.e. as https://github.com/tokio-rs/tokio-proto/pull/183 shows), and is what I'm running with for my development at present.

[carllerche]

It is required to be Send + Sync in order to support multi treading. Fixing this would require a breaking change I believe.

That aid, as a result of the Tokio reform RFC. tokio-proto is going to be significantly simplified and focus more on getting started than provide a full blown solution to all cases. Odds are, in this case, a multi threaded server will be out of scope or at least decoupled in a way to not impose this bound.

[alex]

What do you mean by "in order to support multi threading"? As I said, a NewService instance is created for each thread and never moved from it, so I think multi-threading is still supported with the change I proposed.

[carllerche]

I'll cc @aturon as IIRC, he lead the effort for this bit of the code.

[alex]

https://github.com/alex/ct-tools/blob/master/src/main.rs#L315-L410 is the code I implemented/copy-pasted from tokio-proto to accomplish what I was after, FWIW