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

Use futures 0.3 API #3358

Merged
merged 11 commits into from
Nov 17, 2019
Merged

Use futures 0.3 API #3358

merged 11 commits into from
Nov 17, 2019

Conversation

bartlomieju
Copy link
Member

This is #3299 by @afinch7 - I'm just trying to get it to green

@bartlomieju
Copy link
Member Author

I had to disable [http] serveTLS possibly related to #3160

@bartlomieju
Copy link
Member Author

bartlomieju commented Nov 16, 2019 •
edited
Loading

@ry @afinch7 this should be now landable - I had to disable only 1 test (standard lib, [http] serveTLS)(should work now). I think we should land this PR and start upgrading dependencies as they switch to new futures API.

@bartlomieju bartlomieju changed the title [WIP] Use std future Use futures 0.3 API Nov 16, 2019
ry
ry reviewed Nov 16, 2019
View reviewed changes
cli/file_fetcher.rs
@@ -119,14 +122,14 @@ impl SourceFileFetcher {
pub fn fetch_source_file_async(
self: &Self,
specifier: &ModuleSpecifier,
) -> Box<SourceFileFuture> {
) -> Pin<Box<SourceFileFuture>> {
Copy link
Member

Choose a reason for hiding this comment

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

What is the Pin for?

ry
ry reviewed Nov 16, 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.

Nice work @afinch7 ! Still combing through this - but I will land it today modulo a serious problem.

ry
ry reviewed Nov 16, 2019
View reviewed changes
cli/ops/io.rs
use futures::Future;
use futures::Poll;
use futures::compat::AsyncRead01CompatExt;
use futures::compat::AsyncWrite01CompatExt;
Copy link
Member

Choose a reason for hiding this comment

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

It's very helpful that they've included this.

ry
ry reviewed Nov 16, 2019
View reviewed changes
cli/compilers/ts.rs
let first_msg_fut = async move {
worker.post_message(req_msg).await.unwrap();
let result = worker.await;
if let Err(err) = result {
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@afinch7 afinch7 mentioned this pull request Nov 17, 2019
Closed
ry
ry approved these changes Nov 17, 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 - thank you very much @afinch7 and @bartlomieju

@ry ry merged commit 8f9a942 into denoland:master Nov 17, 2019
@kevinkassimo kevinkassimo mentioned this pull request Nov 17, 2019
Merged
@trivikr trivikr mentioned this pull request Nov 17, 2019
Closed
This was referenced Nov 17, 2019
Merged
Closed
ry pushed a commit that referenced this pull request Nov 17, 2019
@bartlomieju @ry
refactor: fixes for futures (#3363)
f356b2b 
After landing #3358 the benchmarks exploded indicating problems with workers and deno_core_http_bench.

This PR dramatically fixes thread/syscall count that showed up on benchmarks. Thread count is not back to previous levels but difference went from hundreds/thousands to about ~50.
@bartlomieju bartlomieju mentioned this pull request Nov 19, 2019
Closed
@bartlomieju bartlomieju deleted the use_std_future branch December 2, 2019 13:21
@bartlomieju bartlomieju mentioned this pull request Dec 7, 2019
Merged
ry pushed a commit that referenced this pull request Dec 7, 2019
@bartlomieju @ry
fix isolate tests (#3459)
e02029c 
Some tests were silently failing after #3358 and #3434 because pool.spawn_ok
was used which doesn't panic on errors. For reference, the failure looked like this:

thread '<unnamed>' panicked at 'assertion failed: match isolate.poll_unpin(cx) { Poll::Ready(Ok(_)) => true, _ => false, }', core/isolate.rs:1408:7
bartlomieju added a commit to bartlomieju/deno that referenced this pull request Dec 28, 2019
@bartlomieju
Use futures 0.3 API (denoland#3358)
88f0c03
bartlomieju added a commit to bartlomieju/deno that referenced this pull request Dec 28, 2019
@bartlomieju
refactor: fixes for futures (denoland#3363)
b534a17 
After landing denoland#3358 the benchmarks exploded indicating problems with workers and deno_core_http_bench.

This PR dramatically fixes thread/syscall count that showed up on benchmarks. Thread count is not back to previous levels but difference went from hundreds/thousands to about ~50.
bartlomieju added a commit to bartlomieju/deno that referenced this pull request Dec 28, 2019
@bartlomieju
fix isolate tests (denoland#3459)
9db17e1 
Some tests were silently failing after denoland#3358 and denoland#3434 because pool.spawn_ok
was used which doesn't panic on errors. For reference, the failure looked like this:

thread '<unnamed>' panicked at 'assertion failed: match isolate.poll_unpin(cx) { Poll::Ready(Ok(_)) => true, _ => false, }', core/isolate.rs:1408:7
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
@bartlomieju @ry
Use futures 0.3 API (denoland/deno#3358)
7140e06
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
@bartlomieju @ry
Use futures 0.3 API (denoland/deno#3358)
0581aaa
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
@bartlomieju @caspervonb
Use futures 0.3 API (denoland/deno#3358)
b974d23
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
@bartlomieju @caspervonb
Use futures 0.3 API (denoland/deno#3358)
8ba3bbf
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@bartlomieju @caspervonb
Use futures 0.3 API (denoland/deno#3358)
aa2d808
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@bartlomieju @caspervonb
Use futures 0.3 API (denoland/deno#3358)
ca0436b
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@bartlomieju @caspervonb
Use futures 0.3 API (denoland/deno#3358)
dece0b7
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
@bartlomieju @denobot
Use futures 0.3 API (denoland/deno#3358)
e73f2ce
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021
@bartlomieju @denobot
Use futures 0.3 API (denoland/deno#3358)
77dfd8e
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.
3 participants
@bartlomieju @ry @afinch7