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

Add await method about issue#7407 #7414

Closed
wants to merge 3 commits into from

Conversation

zorori777
Copy link

Problem

issue

Analysis

When I updated this pakcage from 10.0.0 to 10.1.0, suddenly happened this error.
So I checked each differences in change log between 10.0.0 to 10.1.0.
ChangeLog

Reason

#7351
It shouldn't remove await method.

This pull request's approval comments say There’s some async stuff happening in the constructor, in the fact constructor call async method. so it need to use await function.

According to error message, when synchronous processing finished, closed entire process at the same time.
Need to wait for asynchronous processing, so I add await method.

Fix

I add await method, removed it by #7351

@google-cla
Copy link

google-cla bot commented Jul 8, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Jul 8, 2021
@@ -433,7 +433,7 @@ export class Browser extends EventEmitter {
url: 'about:blank',
browserContextId: contextId || undefined,
});
const target = this._targets.get(targetId);
const target = await this._targets.get(targetId);
Copy link

Choose a reason for hiding this comment

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

Not sure whether you've looked into the source code but this._targets is a map of Target instances. The assignment happens here:

const target = new Target(
targetInfo,
context,
() => this._connection.createSession(targetInfo),
this._ignoreHTTPSErrors,
this._defaultViewport
);
assert(
!this._targets.has(event.targetInfo.targetId),
'Target should not exist before targetCreated'
);
this._targets.set(event.targetInfo.targetId, target);

The target's constructor might be doing some async work, but you can't mark a constructor as async and await the class instantiation (i. e. await new Target(...) is not possible). The instance will be immediately returned and the constructor's async work happens afterwards in the background.

Did you actually try building puppeteer with this change to see whether this fixes the issue for you?


Edit: the only thing I could imagine is that adding the await would defer this line to be executed later (i. e. pushes it to the end of the event loop). I just did a search and it seems like that's how it works according to this SO answer ("if you await things you will always defer the continuation to a microtask"). TIL 🤓

@mathiasbynens referring to #7351 (comment), is it possible that the await deferring did actually have an effect?

@jschfflr
Copy link
Contributor

I feel like this patch might not really be fixing the problem because the next line is already awaiting target._initializedPromise. The original problem is probably timing related and therefore flaky.
Could you verify that this change is actually fixing the problem?

@jschfflr
Copy link
Contributor

@zorori777 I'll close this for now as there hasn't been an update on this issue. Feel free to reopen if you think this is still relevant!

@jschfflr jschfflr closed this Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants