Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

[dev] fix windows builds and timeouts #991

Merged
merged 11 commits into from
Jan 29, 2020

Conversation

EverlastingBugstopper
Copy link
Contributor

This fixes the issues we were having with logs not working on windows!

@EverlastingBugstopper
Copy link
Contributor Author

EverlastingBugstopper commented Jan 9, 2020

keepalive timer requires that we send a heartbeat message every so often and i'm gonna need tokio-tungstenite for that. probably for the best as it seems to be the recommended websocket library and their master branch has support for async/await

@EverlastingBugstopper EverlastingBugstopper changed the title Switch from ws-rs to tungstenite-rs [dev] fix windows builds and timeouts Jan 9, 2020
@EverlastingBugstopper
Copy link
Contributor Author

One thing I have a question about is my use of two separate tokio runtimes here. I'm wondering if there's a better pattern I should be using.

@EverlastingBugstopper
Copy link
Contributor Author

I think this might fix #888

@steveklabnik
Copy link
Contributor

Yeah, two runtimes aren't ideal. I haven't dug into the code yet but https://docs.rs/tokio/0.2.9/tokio/task/fn.spawn.html is probably what you'd do

@EverlastingBugstopper
Copy link
Contributor Author

So it turns out this builds on Windows but seems to not read incoming socket messages. I've opened this issue to see if the awesome folks over at tokio-tungstenite have any ideas.

@EverlastingBugstopper
Copy link
Contributor Author

@steveklabnik re: your comment about spawn

The following code starts and ends the process almost immediately, it seems to spawn the two tasks and then exit.

    let mut runtime = TokioRuntime::new().unwrap();

    // listen for devtools messages
    runtime.spawn(async move {
        socket::listen(session_id).await
    });

    // handle incoming HTTP requests
    runtime.spawn(async move {
        serve(server_config, preview_id).await
    })?;

The following code seems to work at first, provides a "Listening" message, but fails to handle any incoming HTTP requests:

    let mut runtime = TokioRuntime::new().unwrap();

    // listen for devtools messages
    runtime.spawn(async move {
        socket::listen(session_id).await
    });

    // handle incoming HTTP requests
    runtime.block_on(async move {
        serve(server_config, preview_id).await
    })?;

@steveklabnik
Copy link
Contributor

I believe that's because of this:
image

that is, your runtime goes out of scope at the end of the function, and so shuts down.

@EverlastingBugstopper
Copy link
Contributor Author

I seem to have cracked the code thanks to some help from the friendly Tokio discord server :) - only one runtime now

Copy link
Contributor

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

This looks good! I have a few questions/comments but in general, I approve :)

let devtools_listener = socket::listen(session_id);
let server = serve(server_config, preview_id);

let runners = futures::future::join(devtools_listener, server);
Copy link
Contributor

Choose a reason for hiding this comment

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

🎊

let runners = futures::future::join(devtools_listener, server);

runtime.block_on(async {
let (_, _) = runners.await;
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to not

let _ =

instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

also, are you sure that ignoring all failures is what you want here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point - I'm going to add some .unwrap() calls to them. Not quite sure if there's a way for me to bubble up those failures through the async fn

Copy link
Contributor

Choose a reason for hiding this comment

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

pub fn block_on<F: Future>(&mut self, future: F) -> F::Output

should return that Result back out of block_on, I think.

Copy link
Contributor Author

@EverlastingBugstopper EverlastingBugstopper Jan 22, 2020

Choose a reason for hiding this comment

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

very interesting - i think i got it

Ok(())
}

async fn keep_alive(tx: futures::channel::mpsc::UnboundedSender<Message>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

might be worth giving this a return type of !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting! I'm not familiar with this syntax and can't seem to find an example of a function returning ! - would you mind elaborating?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

(loop has the type !, but since ! coerces to any other type, it coerces to the return type of () here, which is why this code works the way it does)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is very cool

let duration = Duration::from_millis(1000 * KEEP_ALIVE_INTERVAL);
let mut interval = time::interval(duration);

let mut id = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

i am interested why this is 2. not because it's wrong, but it feels unusual! Might deserve a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point - the reason it's 2 is because we have already sent id = 1 earlier to enable the runtime. Eventually I plan on making the ID handling automatic and in the chrome-devtools-rs library.

@EverlastingBugstopper
Copy link
Contributor Author

thanks steve!

@EverlastingBugstopper EverlastingBugstopper merged commit e585c20 into avery/a-time-sync Jan 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the avery/totally-metal branch January 29, 2020 16:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants