refactor: Start translating server.rs to async await by Marwes · Pull Request #787 · mozilla/sccache

Marwes · 2020-06-08T15:40:46Z

No description provided.

froydnj · 2020-06-09T20:30:51Z

This is really interesting, because we kind of thought there was no good way to incrementally transition to new futures. I assume you basically sprinkle .compat() everywhere and then at some point you can go through and remove all of them?

@luser is this basically what y'all did internally?

luser · 2020-06-09T21:20:35Z

We don't use tokio except for some testing tools so it wasn't as much of a hassle for us. Looks like that's basically what we did for those, though.

Marwes · 2020-06-10T07:01:25Z

Yes. My preferred way is to start from the top, replacing things with async/await/std::fuuture::Future and calling compat on any old futures. Then this can repeated, pushing the compat calls deeper into the stack until finally they are completely gone. (By starting from the top it is possible to replace cloning by borrowing in many cases).

glandium

Thank you a lot. I wish it would have been done in smaller increments to ease review, but that's what I failed to do, so 🤷 . Not mentioned in the inline comments: there's a warning about #[macro_use] that you removed in a subsequent patch that should be part of this one.

glandium · 2020-06-10T07:43:50Z

Cargo.toml

 blake3 = "0.1.5"
 byteorder = "1.0"
 bytes = "0.4"
+bytes_05 = { version = "0.5", package = "bytes" }


bytes 0.4 is actually unused since 17c56c8, so you don't need the suffix.

glandium · 2020-06-10T07:48:36Z

src/server.rs

-                }
-            })?;
+            .block_on_std(async { time::timeout(Duration::new(30, 0), wait).await })
+            .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;


This triggers a warning that should be avoided in this patch rather than later:

594 | / runtime 595 | | .block_on_std(async { time::timeout(Duration::new(30, 0), wait).await }) 596 | | .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?; | |___________________________________________________________________^ | = note: `#[warn(unused_must_use)]` on by default = note: this `Result` may be an `Err` variant, which should be handled

You're kind of addressing this in the last patch, but it feels like it should be addressed here instead. That part of the other patch is too much repeating too. How about changing the Output for WaitUntilZero to () instead of io::Result<()>?
It also looks like for some reason (not sure where this is happening), there's an impl From<tokio::time::Elapsed> for std::io::Error, and removing the map_err works.

glandium · 2020-06-10T08:05:23Z

src/server.rs

    }
 }

+struct BincodeCodec;


github doesn't allow to comment on unpatches sections but there's a compile failure in impl<R> futures_03::Stream for Body<R> because the receiver changed type and doesn't have poll anymore. It does have poll_next, though, which simplifies the implementation.

glandium · 2020-06-10T08:05:39Z

src/server.rs

+
+    fn serialize(self: Pin<&mut Self>, item: &T) -> std::result::Result<Bytes, Self::Error> {
+        let mut bytes = BytesMut::new();
+        use bytes_05::buf::BufMutExt;


This can go at the top of the file.

glandium · 2020-06-10T08:11:28Z

src/server.rs

-        Ok(msg.map(Message::WithoutBody).into())
+    fn poll_next(mut self: Pin<&mut Self>, cx: &mut task::Context<'_>) -> Poll<Option<Self::Item>> {
+        Poll::Ready(
+            ready!(Stream::poll_next(Pin::new(&mut self.inner), cx))


Is it more idiomatic than Pin::new(&mut self.inner).poll_next(cx)?

glandium · 2020-06-10T08:20:54Z

src/server.rs

-            io::Error::new(io::ErrorKind::Other, e)
-        }));
-        Ok(msg.map(Message::WithoutBody).into())
+    fn poll_next(mut self: Pin<&mut Self>, cx: &mut task::Context<'_>) -> Poll<Option<Self::Item>> {


task::Context is a reexport of std::task::Context, which is imported already, please just use Context.

glandium · 2020-06-10T08:21:53Z

src/server.rs

+    future,
+    prelude::*,
+    ready, stream,
+    task::{self, Poll},


Poll is a reexport of std::task::Poll, please import Poll from there instead. As a follow-up, we can replace std::task::Poll with Poll in this file.

glandium · 2020-06-10T08:23:20Z

src/server.rs

                // Shutdown received!
-                Async::Ready(Some(ServerMessage::Shutdown)) => return Ok(().into()),
-                Async::Ready(Some(ServerMessage::Request)) => {
+                Poll::Ready(Some(ServerMessage::Shutdown)) => return ().into(),


return Poll::Ready(())?

glandium · 2020-06-10T08:23:38Z

src/server.rs

                }
                // All services have shut down, in theory this isn't possible...
-                Async::Ready(None) => return Ok(().into()),
+                Poll::Ready(None) => return ().into(),


glandium · 2020-06-10T08:26:59Z

src/server.rs

-                .poll()
-                .map_err(|err| io::Error::new(io::ErrorKind::Other, err)),
+            None => Poll::Pending,
+            Some(ref mut timeout) => ready!(Pin::new(timeout).poll(cx)).into(),


Is the ready!().into() necessary? I now wonder if that's what was missing in my failed attempt at converting this impl.

(It wasn't the lack of ready! that made my own attempt fail)

glandium · 2020-06-10T08:33:19Z

src/errors.rs

        TempfilePersist(tempfile::PersistError);
        WalkDir(walkdir::Error);
        Timer(tokio_timer::Error);
+        Timeout(tokio::time::Elapsed);


This is likely to lose the race against #777 so you may want to rebase on top of that one after it's merged.

glandium · 2020-06-10T08:43:12Z

src/server.rs

    type Response = SccacheResponse;
    type Error = Error;
-    type Future = SFuture<Self::Response>;
+    type Future = Pin<Box<dyn Future<Output = Result<Self::Response>>>>;


A SFuture03 alias would be valuable.

glandium · 2020-06-10T08:48:28Z

src/server.rs

-                    Message::WithoutBody(Response::ShuttingDown(Box::new(info)))
-                }));
+        let self_ = self.clone();
+        Box::pin(async move {


I'm not particularly convinced using async here is better than not.

Why? This allows await to be used, making it more flexible.

It moves some code into a future, and as of right now, I'm not sure this is desired.

glandium · 2020-06-10T08:54:50Z

src/server.rs

+                let r: Result<Box<dyn Compiler<C>>> = info.map(|info| info.0);
+                r
+            }
+        }


Less indentation FTW.

glandium · 2020-06-10T08:55:44Z

src/server.rs

+                cache_control,
+                self.pool.clone(),
+            )
+            .compat();


I really hate what rustfmt does when you add .compat(). Not much you can do about though (although you could probably do something like let result = ...; let result = result.compat();). It's just the nth I see and I had to say it.

compat calls are just temporary 🤷

glandium · 2020-06-10T08:58:25Z

src/server.rs

            res.color_mode = color_mode;
            match result {
                Ok((compiled, out)) => {
+                    let mut stats = me.stats.borrow_mut();


There's only one Error branch that doesn't use stats. I don't feel it needs to be moved.

I had to move it to prevent it from living across an await

Moving it back to where it was compiled just fine for me.

It may compile fine, but it can still break because another future accessing the RefCell may run if the future yields in the await

Marwes · 2020-06-10T11:52:28Z

Should have rebased and squashed these commits before making the PR, don't want to do it now since it messes up reviews.

glandium

Can you rebase on top of #792, and squash things, but still separate the tokio-0.2 and the async-await parts?

glandium · 2020-06-11T01:02:20Z

Cargo.lock

 "bincode",
 "blake3",
 "byteorder",
- "bytes 0.4.12",


Why does this not remove the corresponding [[package]] section?

There are still other dependencies that require bytes 0.4

glandium

The nits for this commit were applied in the second commit :(

glandium self-requested a review June 10, 2020 01:20

glandium requested changes Jun 10, 2020

View reviewed changes

glandium requested changes Jun 11, 2020

View reviewed changes

Marwes force-pushed the async_server branch 2 times, most recently from 0113664 to 0a3d55e Compare June 11, 2020 08:29

glandium requested changes Jun 11, 2020

View reviewed changes

Marwes force-pushed the async_server branch from 0a3d55e to 8037870 Compare June 12, 2020 07:56

refactor: Convert server.rs to use async await

db2629d

Marwes force-pushed the async_server branch from 8037870 to db2629d Compare June 15, 2020 07:26

drahnr mentioned this pull request Nov 12, 2020

refactor: Convert server.rs to use async await paritytech/cachepot#5

Merged

glandium added this to the 0.3.0 milestone Jan 8, 2021

Marwes closed this Feb 5, 2021

Marwes mentioned this pull request Feb 5, 2021

refactor: Convert server.rs to use async await #945

Closed

Comments

Conversation

Marwes commented Jun 8, 2020

Uh oh!

froydnj commented Jun 9, 2020

Uh oh!

luser commented Jun 9, 2020

Uh oh!

Marwes commented Jun 10, 2020

Uh oh!

glandium left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glandium Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marwes commented Jun 10, 2020

Uh oh!

glandium left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glandium left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

glandium Jun 11, 2020 •

edited

Loading