Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update to std::future::Future #265

Merged
merged 2 commits into from Oct 24, 2019
Merged

update to std::future::Future #265

merged 2 commits into from Oct 24, 2019

Conversation

jxs
Copy link
Collaborator

@jxs jxs commented Aug 26, 2019

No description provided.

@jxs jxs force-pushed the master branch 2 times, most recently from 3e73867 to 23623d3 Compare September 16, 2019 13:15
@dbcfd
Copy link

dbcfd commented Sep 16, 2019

async tokio-tungstenite for websocket support snapview/tokio-tungstenite#68

@jxs jxs force-pushed the master branch 5 times, most recently from a22ccc9 to bd33766 Compare September 17, 2019 14:28
src/filter/or.rs Outdated
return Err(e.combine(err1));
}
pin.original_path_index.reset_path();
pin.state = State::Second(Some(err1), fut2);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loop seems to be breaking tests/path.rs#or

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that one is solved thanks
some more to go 😀

@jxs jxs force-pushed the master branch 6 times, most recently from ac9bc57 to 2b18b7f Compare September 18, 2019 12:59
@zimond
Copy link

zimond commented Sep 20, 2019

There is a way to bring the websocket module back I think... tokio-tungstenite

@bstrie
Copy link

bstrie commented Sep 20, 2019

We literally just switched to warp to take advantage of its websocket support, so I'd love if that could stay first-class in the long run. :)

Cargo.toml Outdated Show resolved Hide resolved
@dbcfd
Copy link

dbcfd commented Sep 20, 2019

We literally just switched to warp to take advantage of its websocket support, so I'd love if that could stay first-class in the long run. :)

The tokio-tungstenite branch I put above seems to be working. Would be great to pull it into this branch and do some more testing on it. I do need to add more tests to it.

let hello = warp::path::end().map(warp::reply);

let oops =
warp::path("oops").and_then(|| Err::<StatusCode, _>(warp::reject::custom(Error::Oops)));
warp::path("oops").and_then(|| async { Err::<StatusCode, _>(warp::reject::custom(Error::Oops))});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than doing an async closure, use futures::future::ready

Err(err)
}
};
async { err }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, use futures::future::ready so we're not paying the generator cost.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! had missed that

fn poll(&mut self) -> Poll<Self::Item, Self::Error> {
self.extract.poll().map_err(|err| (self.callback)(err))
fn poll(mut self: Pin<&mut Self>, cx: &mut Context) -> Poll<Self::Output> {
let extract = &mut get_unchecked!(self).extract;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly use pin project to get a pinned field to poll?

@jxs jxs changed the title WIP: update to std::future::Future update to std::future::Future Sep 25, 2019
Cargo.toml Outdated
headers = "0.2"
http = "0.1"
hyper = "0.12.18"
hyper = "=0.13.0-alpha.1"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hyper alpha 2 is released. I think here we can upgrade this dependency and just add some modifications in server code to adapt to the new version zimond@139a5ab

Copy link
Collaborator Author

@jxs jxs Sep 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! had to also implement Accept due to AddrIncoming no longer implementing Stream

@jxs jxs force-pushed the master branch 2 times, most recently from 1a47d0f to 3e07dfd Compare September 27, 2019 18:19
if let Err(err) = result {
log::error!("server error: {}", err)
}
});
Copy link
Contributor

@udoprog udoprog Sep 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This swallows any errors caused by binding while previously we used to return a Result with Err = (). That's because old futures are equivalent to Future<Output = Result<T, E>>. With this, the server future is replaced with one that immediately yields when failing to bind which is probably not what we want.

To mimic old behavior, we can go back to using map_err. But it's probably better to rewrite this whole shebang to be .await:ed directly since we're making a breaking change anyways.

Copy link
Contributor

@udoprog udoprog Sep 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a mistake. Binding is still synchronous through hyper and isn't affected by this change!

if let Err(err) = result {
log::error!("server error: {}", err)
}
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Cargo.toml Outdated
rustls = { version = "0.15", optional = true }
tokio-threadpool = "0.1.7"
tokio = "0.2.0-alpha.5"
tokio-executor = "=0.2.0-alpha.5"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

time to bump tokio: 0.2.0-alpha.6 :)

@udoprog
Copy link
Contributor

udoprog commented Oct 6, 2019

I've rebased and bumped dependencies here because I need it:
https://github.com/udoprog/warp

I accidentally ran cargo fmt while working so I guess we have that now as well 🤷‍♂ . Would be nice if we could make that consistent in the codebase (@seanmonstar ?).

@lpil
Copy link

lpil commented Oct 17, 2019

Great stuff! Is there anything we can do to help get this over the line?


/// Server-sent event message
pub trait ServerSentEvent: SseFormat + Sized + Send + 'static {
pub trait ServerSentEvent: SseFormat + Sized + Send + Sync + 'static {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for the Sync bound? Pre std::Future SSE replys did not have to be Sync.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now see why. It's a requirement from hyper.

@seanmonstar
Copy link
Owner

This is super exciting, as I've said before. I've given @jxs merge rights, so when it's ready and green, we can merge :D

@NeoLegends
Copy link

NeoLegends commented Oct 24, 2019

So the only requirement for now is waiting until async await hits stable? Tests are green in beta, where a/a is already available without feature gates.

@udoprog
Copy link
Contributor

udoprog commented Oct 24, 2019

We're two weeks out from a stable async/await. Building on beta is a very strong indicator that it will work by then, so I personally wouldn't mind merging to master right now. Note that I've also run this branch in staging for a while without incident.

@jxs jxs merged commit 5c26956 into seanmonstar:master Oct 24, 2019
@jxs jxs mentioned this pull request Oct 24, 2019
warp::spawn(poll_fn(move || dtx.poll_cancel()).map(move |_| {
warp::spawn(async move {
dtx.closed().await;
drx.close();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That usage hold the oneshot::channel open forever, so the fn user_disconnected(...) not reachable.

@bstrie
Copy link

bstrie commented Jan 8, 2020

The tokio-tungstenite branch I put above seems to be working. Would be great to pull it into this branch and do some more testing on it. I do need to add more tests to it.

@dbcfd Took a minute, but I see that snapview/tokio-tungstenite#68 has finally landed. :) You were suggesting that we replace Warp's usage of tungstenite with tokio-tungstenite in order to fully complete the async transition, yes?

@jxs
Copy link
Collaborator Author

jxs commented Jan 8, 2020

I am going to open a separate issue, so it's easier to track: #366

@dbcfd
Copy link

dbcfd commented Jan 8, 2020

@bstrie Yeah, I would recommend using snapview/tokio-tungstenite#68. We encountered a few issues during the upgrade around context and waking, but they have been worked through, and it should be failure solid now. Hopefully easy to integrate as well, since the previous warp implementation was close to the tokio-tungstenite implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants