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

Fix FluentWait so it completes in more cases. #7692

Closed
wants to merge 4 commits into from

Conversation

utamas
Copy link
Contributor

@utamas utamas commented Oct 16, 2019

FluentWait could potentially never complete when the condition
passed to it made no progress (either because it fall into an
infinite loop or waiting for a resource or response over
network).

Fixes #7494

@utamas
Copy link
Contributor Author

utamas commented Oct 16, 2019

@barancev Could you please review this PR?

FluentWait could potentially never complete when the condition
passed to it made no progress (either because it fall into an
infinite loop or waiting for a resource or response over
network).

Fixes SeleniumHQ#7494
Hanged is a dodgy expression.

Fixes SeleniumHQ#7494
@utamas
Copy link
Contributor Author

utamas commented Nov 5, 2019

@barancev : could you please give an insight on how PRs get processed? I can imagine that you guys are extremely busy and 20 days is not the end of the world to wait for a PR to be reviewed. It's just killing my excitement to try to do any further contribution not to know what to expect and when.

@shs96c
Copy link
Member

shs96c commented Nov 12, 2019

I appreciate this is not the best experience with the project. We really are very grateful for the PR. Right now, there are two people active in the java tree: @barancev and @shs96c (me!), and both of us are a little overwhelmed. Thank you for your patience, and the nudge to get things going.

The review process is a little ad-hoc, but I'll speak for myself. I try and scan the backlog of PRs when I get a chance (which, I'll be honest, hasn't been often recently). If I can review it quickly, and merge it, I'll do that. If the PR requires a longer review, I tend to set it aside for a time when I have enough time to do a proper job. A gentle nudge on the Slack channel is a great way to get one of us to focus on your PR if we're being super-slow.

Yes, I know that this isn't the best way to run a project, but I'm generally working on Selenium in my free time, and there's not been a lot of that lately. :(

@utamas
Copy link
Contributor Author

utamas commented Nov 14, 2019

Thank you @shs96c for the insight and for your work on this project.

@CLAassistant
Copy link

CLAassistant commented Nov 23, 2019

CLA assistant check
All committers have signed the CLA.

@utamas
Copy link
Contributor Author

utamas commented Nov 25, 2019

I'll try to resolve the test failures

@barancev
Copy link
Member

barancev commented Mar 8, 2020

Merged as 51de536, thank you!

@barancev barancev closed this Mar 8, 2020
@barancev
Copy link
Member

Sorry to say this, but we decided to return back to the legacy (blocking) implementation after clients complains. It appears that many people use suboptimal approach to manage WebDriver instances using TreadLocal variables. And running conditions in a separate thread makes them lost their driver. So we decided to revert this change in favour of backward compatibility.

People who want to use FluentWait for potentially blocking conditions should create an alternative class that uses a sort of ExecutorService, and this PR can be used as a draft for this new class.

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.

FluentWait might never complete
4 participants