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.

Sign up for GitHub

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

Jump to bottom

remove tokio_util::block_on #3388

Merged
merged 7 commits into from
Nov 22, 2019
Merged

remove tokio_util::block_on #3388

merged 7 commits into from
Nov 22, 2019

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Nov 20, 2019 •
edited
Loading

This PR removes tokio_util::block_on - refactored compiler and file fetcher slightly so that we can safely block there - that's because only blocking path consist of only synchronous operations.

Additionally I removed excessive use of tokio_util::panic_on_error and tokio_util::run_in_task and moved both functions to cli/worker.rs, to tests module.

Closes #2960

@bartlomieju
Copy link
Member Author 

"thread_count": {
    "error_001": 3, 
    "text_decoder": 3, 
    "cold_relative_import": 3, 
    "cold_hello": 3, 
    "workers_round_robin": 4, 
    "relative_import": 3, 
    "workers_startup": 4, 
    "hello": 3
  },

CC @ry

@95th
Copy link
Contributor

95th commented Nov 21, 2019

We can't actually remove it (instead of shifting to futures block_on), can we?

@bartlomieju
Copy link
Member Author

Well @95th that's what I wanted to do in this PR but Windows is acting up... I don't have a Windows machine to debug it

@bartlomieju bartlomieju mentioned this pull request Nov 22, 2019
Closed
ry
ry reviewed Nov 22, 2019
View reviewed changes
cli/tokio_util.rs
/// This is useful when we want to block the main runtime to
/// resolve a future without worrying that we'll use up all the threads in the
/// main runtime.
pub fn block_on<F, R>(future: F) -> Result<R, ErrBox>
Copy link
Member

Choose a reason for hiding this comment

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

Very happy to see this go away. I think we still haven't really solved the core problem - but much better to be using futures::executor::block_on rather than this home-brewed garbage. Nice work!

ry
ry approved these changes Nov 22, 2019
View reviewed changes
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM!

@ry ry merged commit c6bb3d5 into denoland:master Nov 22, 2019
@bartlomieju bartlomieju deleted the remove_tokio_util_block_on branch November 22, 2019 17:51
bartlomieju added a commit to bartlomieju/deno that referenced this pull request Dec 28, 2019
@bartlomieju
remove tokio_util::block_on (denoland#3388)
dff2b76 
This PR removes tokio_util::block_on - refactored compiler and file 
fetcher slightly so that we can safely block there - that's because 
only blocking path consist of only synchronous operations.

Additionally I removed excessive use of tokio_util::panic_on_error 
and tokio_util::run_in_task and moved both functions to cli/worker.rs, 
to tests module.

Closes denoland#2960
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ry ry ry approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Never create more than one Tokio runtime. Remove tokio_util::block_on
3 participants
@bartlomieju @95th @ry