Changes the return value of Connection::into_parts in the server upgrade API

[ubnt-intrepid]

In the upgrade API provided by the current master, the fields of Parts<T> returned from into_parts() are only the underling I/O and buffer. In order to pass the context genereted within the Service::call to the task of lower layer, it is necessary to use the message passing using the channel as follows:

// A pair of mpsc channel to pass a value of context.
let (tx, rx) = futures::sync::mpsc::unbounded();

let service = service_fn(move |req| {
     ...
    // send the value of context to the following connection task
    if some_condition(req) {
        tx.unbounded_send(context);
    }
    Ok(response)
});
let conn = protocol.serve_connection(io, service);

// task to manage a TCP connection considering upgrade
let mut conn_opt = Some(conn);
let mut rx_opt = Some(rx);
let task = poll_fn(move || {
    try_ready!(conn_opt.as_mut().unwrap().poll_without_shutdown());

    let conn = conn.take().unwrap();
    let Parts { io, read_buf, .. } = conn.into_parts();

    // Try to retrieve the value of the context.
    let mut rx = rx_opt.take().unwrap();
    match rx.poll().unwrap() {
        Ok(Async::Ready(Some(context))) => {
            // migration to upgraded protocol
        },
        _ => {
            // shutdown I/O
        }
    }
});

Using such a method complicates the connection between the Service layer and the lower connection task. To avoid this, the modification of the upgrade API by adding additional field to the return value from Connection::into_parts() is required.

My suggestion is to add the value of Service used in HTTP as follows:

#[derive(Default)]
struct MyService {
    // such internal mutability will become unnecessary
    // by changing the receiver of Service::call to &mut self.
    context: RefCell<Option<UpgradeContext>>,
}

impl Service for MyService {
    fn call(&self, req: Request) -> Self::Future {
        // set the value of context
        if some_condition(req) {
            *context.borrow_mut() = Some(context);
        }
        Ok(response)
    }
}
let conn = protocol.serve_connection(io, MyService::default());

let mut conn_opt = Some(conn);
let task = poll_fn(move || {
    try_ready!(conn_opt.as_mut().unwrap().poll_without_shutdown());

    // There is no additional channel to receive the value of context.
    let conn = conn.take().unwrap();
    let Parts { io, read_buf, service, .. } = conn.into_parts();
    match service.context {
        Some(context) => {
            // migration to upgraded protocol
        },
        _ => {
            // shutdown I/O
        }
    }
});

Is it possible to make such changes?

[seanmonstar]

Hm, I suppose on the server side, it should be possible to return the service with into_parts, yes.

Care must be taken by the user since the service manages every request on the connection, and so bugs could exist if you set state on your service incorrectly, but I don't think this change makes this bug any easier to do. You'd have the same possibility with the channel example that you showed initially.

The good news is that the into_parts feature hasn't been release on crates.io yet, so changing to Parts<T, S> wouldn't be a breaking change.

[ubnt-intrepid]

It is right that the attention to implementation is required.

I've also noticed that this method cannot deal with the context generated after calling Service::call (i.e. generated as a resolved value of Service::Future). As in the above example, it could be solve the problem by passing the value of Future held in in_flight, but I think this is not a smart solution.

A better way is that Response<T> (the resolved value from Future) has an additional field. It may occur some breaking changes involving http crate.

[seanmonstar]

I think it's probably best to not try to address that possible issue yet, and see if it is a problem in practice. If it is, we could do something to employ Response::extensions(), but again, I'd leave that for later.

[ubnt-intrepid]

I've understood. Continually use the current API and identify the problems.

[seanmonstar]

This was done and is part of 0.11.24.