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

Implement page closing #12

Merged
merged 2 commits into from
Dec 28, 2020
Merged

Implement page closing #12

merged 2 commits into from
Dec 28, 2020

Conversation

shulcsm
Copy link
Contributor

@shulcsm shulcsm commented Dec 26, 2020

Would be nice to have a way to just close the target but i couldn't figure it out.

@mattsse
Copy link
Owner

mattsse commented Dec 28, 2020

Thanks for this!
I wasn't sure how to handle a page close command, in particularly how to make sure that PageInner gets dropped.
However the a Page.Close event eventually destroys the target which drops the receiver half of the channel.
The problem right now is that the Page.close command returns before the target is destroyed, making it possible to send commands to the browser for a target that is being destroyed.
This behavior is fine for now, but needs more work, either delaying the Page.close command until Target.targetdestroyed was fired or add a designated Handlermessage variant to remove the target as soon as the Page.close is fired.

src/page.rs Outdated Show resolved Hide resolved
src/page.rs Outdated Show resolved Hide resolved
@shulcsm
Copy link
Contributor Author

shulcsm commented Dec 28, 2020

Thanks for this!
I wasn't sure how to handle a page close command, in particularly how to make sure that PageInner gets dropped.
However the a Page.Close event eventually destroys the target which drops the receiver half of the channel.
The problem right now is that the Page.close command returns before the target is destroyed, making it possible to send commands to the browser for a target that is being destroyed.
This behavior is fine for now, but needs more work, either delaying the Page.close command until Target.targetdestroyed was fired or add a designated Handlermessage variant to remove the target as soon as the Page.close is fired.

I'm not sure if waiting for Target.targetdestroyed is necessary if assuming target eventually gets cleaned up. Page gets consumed and if you are dealing with raw ids you are on your own.

I think it's more important to figure out a way to just destroy the page target since for majority of cases you don't care about unload and just want to clean up.

@shulcsm shulcsm requested a review from mattsse December 28, 2020 11:28
@mattsse
Copy link
Owner

mattsse commented Dec 28, 2020

I'm not sure if waiting for Target.targetdestroyed is necessary if assuming target eventually gets cleaned up. Page gets consumed and if you are dealing with raw ids you are on your own.

Agree. But there is a potential race condition for following scenario:

let el =  page.find_element("input#searchInput").await?;
...
page.close().await?;
...
// this will fail because page is already closed.
el.click().await?;

This el.click() will fail regardless, but at the moment the reason it fails is because the click command is sent to the browser and the browser returns an error. This happens because the Target of the Page is not removed yet and is still able to receive events sent from an Element. Preferably the el.click() should fail with a SendError because the receiver should already be dropped at this point, preventing the overhead of sending a request that is guaranteed to fail.

I think it's more important to figure out a way to just destroy the page target since for majority of cases you don't care about unload and just want to clean up.

This can be done with Target.closeTarget instead, that's how puppeteer does it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants