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

Fixes WaitForNavigation race condition in #125 #281

Merged
merged 1 commit into from
May 10, 2019
Merged

Fixes WaitForNavigation race condition in #125 #281

merged 1 commit into from
May 10, 2019

Conversation

ap0
Copy link
Contributor

@ap0 ap0 commented Apr 10, 2019

When multiple redirects happen very quickly, sometimes the event handler will not be removed before the next page is loaded, so it will call the handler twice and panic because it's attempting to close a previously-closed channel.

Closes #125

@codecov
Copy link

codecov bot commented Apr 10, 2019

Codecov Report

Merging #281 into master will increase coverage by <.1%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master    #281     +/-   ##
========================================
+ Coverage      56%   56.1%   +<.1%     
========================================
  Files         189     189             
  Lines        5777    5777             
========================================
+ Hits         3238    3240      +2     
  Misses       2193    2193             
+ Partials      346     344      -2

@ziflex
Copy link
Member

ziflex commented Apr 10, 2019

Good use case. I wonder how stable it is in this case. If many redirects occur, it handles only first one, it misses the rest which might also lead to some unpredictable behavior.

@ap0
Copy link
Contributor Author

ap0 commented Apr 11, 2019

Yeah, I don't know enough about how this works to say. Right now, though, it crashes whenever it encounters this. Would it be better to somehow have the event broker map events to specific pages when it loads instead?

@ziflex
Copy link
Member

ziflex commented Apr 11, 2019

What if we create a debounce function to handle that?
Something like this:
https://gist.github.com/jonstaryuk/133ced9882aa00502fa7d1db31cb5253

@ap0
Copy link
Contributor Author

ap0 commented Apr 12, 2019

I like that solution, but what about the case where it's in the middle of a redirect when the debounce function finally calls the callback (i.e. one of the intermediate pages is slow but will still redirect)?

The big thing for me here is to just guard against calling close() on a channel twice, which I don't think that would guarantee, but would make the chances of it happening less likely.

@ziflex
Copy link
Member

ziflex commented Apr 12, 2019

Good point. I think in this case it may just fallback to this initial solution with sync.Once.

@ziflex ziflex added area/drivers HTML drivers area/drivers/cdp Cdp driver labels Apr 12, 2019
@ziflex
Copy link
Member

ziflex commented May 10, 2019

Ok, I will merge it. But we need to improve it in the future.

@ziflex ziflex merged commit 65b6981 into MontFerret:master May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/drivers/cdp Cdp driver area/drivers HTML drivers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WaitForNavigation callback can get called more than once.
2 participants