Iterator should reload the collection on every loop cycle

[slipwalker]

With #696 that was introduced in Selenide version 4.12.3, some of our UI autotests that have deals with Selenide's setValue() method started suddenly to fail with next exception:

Element not found {$$(4 elements)[2]}
Expected: exist
Timeout: 4 s.
Caused by: StaleElementReferenceException: stale element reference: element is not attached to the page document
	at com.codeborne.selenide.impl.WebElementSource.createElementNotFoundError(WebElementSource.java:31)
	at com.codeborne.selenide.impl.CollectionElement.createElementNotFoundError(CollectionElement.java:42)
	at com.codeborne.selenide.impl.SelenideElementProxy.dispatchAndRetry(SelenideElementProxy.java:119)
	at com.codeborne.selenide.impl.SelenideElementProxy.invoke(SelenideElementProxy.java:65)
	at com.sun.proxy.$Proxy124.findElements(Unknown Source)
	at com.codeborne.selenide.impl.WebElementSelector.findElements(WebElementSelector.java:38)
	at com.codeborne.selenide.impl.BySelectorCollection.getElements(BySelectorCollection.java:29)
	at com.codeborne.selenide.impl.CollectionElement.createElementNotFoundError(CollectionElement.java:39)
	at com.codeborne.selenide.impl.WebElementSource.checkCondition(WebElementSource.java:59)
	at com.codeborne.selenide.commands.Should.should(Should.java:35)
	at com.codeborne.selenide.commands.Should.execute(Should.java:29)
	at com.codeborne.selenide.commands.Should.execute(Should.java:12)
	at com.codeborne.selenide.commands.Commands.execute(Commands.java:145)
	at com.codeborne.selenide.impl.SelenideElementProxy.dispatchAndRetry(SelenideElementProxy.java:90)
	at com.codeborne.selenide.impl.SelenideElementProxy.invoke(SelenideElementProxy.java:65)
	at com.sun.proxy.$Proxy124.should(Unknown Source)
	at com.codeborne.selenide.impl.ElementFinder.createElementNotFoundError(ElementFinder.java:88)
	at com.codeborne.selenide.impl.WebElementSource.checkCondition(WebElementSource.java:59)
	at com.codeborne.selenide.impl.WebElementSource.findAndAssertElementIsVisible(WebElementSource.java:72)
	at com.codeborne.selenide.commands.SetValue.execute(SetValue.java:32)
	at com.codeborne.selenide.commands.SetValue.execute(SetValue.java:15)
	at com.codeborne.selenide.commands.Commands.execute(Commands.java:145)
	at com.codeborne.selenide.impl.SelenideElementProxy.dispatchAndRetry(SelenideElementProxy.java:90)
	at com.codeborne.selenide.impl.SelenideElementProxy.invoke(SelenideElementProxy.java:65)
	at com.sun.proxy.$Proxy124.setValue(Unknown Source)

Element we trying to set value to is having validation (red border in case of empty value). Element is present and displayed before passing it to setValue() method. So it may look like collection containing that element becomes stale when element's validation occurs (when clear() action happens within setValue() method). But, everything works perfect for this test when rollback to previous version. Cannot share tests code, but will try to do my best to reproduce it somewhere else as well.

[kalumpiwarra]

Hello @asolntsev ,
I can provide an example, which demonstrate this issue:

Selenide.open("http://the-internet.herokuapp.com/challenging_dom");
$$x(".//a[contains(@class,'button')]").forEach(SelenideElement::click);

It's not a perfect example, but it's demonstrate an issue.
One more way to reproduce:

Selenide.open("http://the-internet.herokuapp.com/challenging_dom");
for (SelenideElement element : $$x(".//a[contains(@class,'button')]")) {
   element.click();
}

Both cases are using iterator which uses fetch() as collection snapshot for faster iteration. But in this cases, we are losing the ability to avoid StaleElementReferenceException in case if an element in DOM was changed.

Possible way to avoid such error is using get(int idx) of collection. Example will work without errors:

Selenide.open("http://the-internet.herokuapp.com/challenging_dom");
ElementsCollection elements = $$x(".//a[contains(@class,'button')]");
for (int idx = 0; idx < elements.size(); idx++) {
    elements.get(idx).click();
}

But in such case we are loosing performance on big collections.

P.S. I faced with such error a lot and proposing support both: reload on each call and lazy ways of working with collections

[slipwalker]

@asolntsev any updates on this one? Provided examples look like valid. Anyway, would be great to have this nasty issue fixed. Thanks in advance.

[asolntsev]

@slipwalker Thank you for the reminder. I was busy with Selenide 5.0.0 major refactoring during last month...

I looked at your examples. There is no a single correct answer, of course, but I tend to think that these are not good tests. Ideally, there should not be any loops and ifs in test. I mean, you should always know exactly how many elements there should be on a page, and click every of them explicitly. Loop is a bad solution: if there is NO elements at all, such test would also be green.

It also seems natural to me to get element again if I know that the page has been reloaded meanwhile. That's why using get(int idx) seems pretty natural to me.
And what performance problem you are talking about? You mean accessing an element by index in ArrayList? Camon, it's the fastest thing in the Universe! It's zillion times faster than any call of any WebDriver method (which is performed by send http request to webdriver).

[kalumpiwarra]

@asolntsev Let me clarify couple details:

1. It's suitable to perform some loops in tests (not directly but in page objects), e.g., click and get a result, and only after it validate the results (do not see here any bad solution). I'm expecting that iteration and get(int idx) will have the same behavior in loop. For now, it's not true.
2. Performance issue: I'm not talking about get(int idx) performance. I'm talking about fetch() in case of iterator (but it's risky, you can face StaleElementReferenceException) VS get(int idx) performance in loop, it's reloading collection for each call (if you have > 180 elements you get low performance compare to fetch()).

p.s. it worked perfectly before reverting some changes in collections (#696)

[illiaChe]

Hi.
I agree with the author and confirm that StaleElement exception is thrown in 100% cases when you are operating with collection via stream and its elements are changed in dom during those. And of course, is not reproduced when you are using plain old cycle, because there element collection is polled each time

And the reason is not new Iterator and fetch() method in ElementsCollection but the fact that WebElementsCollectionWrapper.getElements() always returns one old Array of elements without getting them from dom again

And I couldn't find any option to avoid this but to write spikes which create 2 new ElementCollections for my locator between some intervals and comparing them with some poll interval.

@asolntsev Using plain old get(int idx) instead of streams looks very unnatural to me on the contrary

[asolntsev]

@TuXuuTT Once again: typically I also prefer stream instead if get(index). But it changes when we are talking about elements that are disappearing (when the page gets reloaded). You cannot iterate a collection which has changing content. You would get ConcurrentModificationException or something like that. In this case, get(index) is the only working option.

[asolntsev]

@kalumpiwarra 1.

I'm expecting that iteration and get(int idx) will have the same behavior in loop

Yes, this is absolutely reasonable. This is the reason why I was in doubt when releasing Selenide 4.12.3 (in which iterator caches the collection)

2. I still don't understand the performance point. You are suggesting to fetch the whole collection on every operation, right? This is definitely slower than fetching the collection only once (and caching it).

To summarize, we have two options:

1. Iterator caches the collection. (pros: faster, cons: could be unexpected) [current behaviour]
2. Iterator reloads the collection on every loop cycle (cons: slower, pros: simple for everybody)

@vinogradoff @BorisOsipov @rosolko @symonk
Probably we should choose option 2? (users who need high performant iteration can use method $$.snapshot())

[rosolko]

@asolntsev As I remember some one from community also have this issue even with cache call. I suggest to introduce option to switch reload strategy and let switch it based on you need.
Linked with #815

[kalumpiwarra]

@asolntsev Second point is about performance because of current and previous implementation of collection's access.

• Before Selenide 4.12.3 we had iteration via collection without reloading of all collection elements for each iteration. A very imprortant point that during access to each element you check that element do not produce StaleElementReference exception and if it was produces you just reload collection and return actual element. I love such a solution because of it safe and fast solution. If I want to receive a new collection I can easily request it via $$().
• Since Selenide 4.12.3 I cannot iterate over collection via standard iterator because it can produce StaleElementReference exception. And it's not due to reloading of the page but e.g. I need to scroll to an element and only after it I can take element's text (for each element). During such actions StaleElementReference exception can be produced (in my case it's ~ 10% cases) because of "raw" access to elements via iterator or snapshot() (without handling of possible StaleElementReference exception). As a workaround I'm getting each element via get(idx) but it's very slow due to reloading of all collection on each call.

That's why I'm proposing support both solutions (reload strategy) as @rosolko mentioned.

[asolntsev]

@rosolko I don't like the idea of introducing "reload strategy" because it makes things more complex. It makes user's life harder if Selenide has too many options.

@kalumpiwarra

• It seems to me that we never implemented behaviour that you described in "Before Selenide 4.12.3" part. At least intentionally :)
• In "Since Selenide 4.12.3" part, I still don't understand how you are getting StaleElementReference because of scrolling. And how another "reloading strategy" can solve this problem. "Reloading strategy" would not scroll anyway, right?

[kalumpiwarra]

@asolntsev

• It seems that "Before Selenide 4.12.3" behavior was as I described. Here is the code:

@Override
  public WebElement getWebElement() {
    try {
      WebElement el = collection.getElements().get(index);
      el.isEnabled(); // check staleness

      return el;
    } catch (StaleElementReferenceException | IndexOutOfBoundsException e) {
      return collection.getActualElements().get(index);
    }
  }

• "Since Selenide 4.12.3" iterator fetching elements and if something was changed in the DOM during iteration it may produce StaleElementReference exception. F.e. in an angular application I'm iteration via elements collection and perform an action with each element (f.e. click on it). One more case is a lot of elements which are not cannot be displayed simultaneously in the browser's view-port and I need scroll to each element. It also may produce StaleElementReference exception because elements can be changed when they appear in view-port.

Another reload strategy can check element state before return it and reload collection if needed (as it was). Compare to fetching collection without the ability to reload (which is not safe and can produce StaleElementReference exception).
The main idea of reload strategy is to support iteration over elements which may produce StaleElementReference exception.

[asolntsev]

@kalumpiwarra @vinogradoff @BorisOsipov @rosolko @symonk

After thinking a while, I suggest to select option 2 from #797 (comment):

Iterator reloads the collection on every loop cycle
(users who need high performant iteration can use method $$.snapshot())

So, we will have 2 options to iterate a collection:

1. $$("input").forEach(SelenideElement::click);
   // reloads elements every time, solves StaleElementReferenceException
2. $$("input").snapshot().forEach(SelenideElement::click);
   // uses cached elements. Fast, but may cause StaleElementReferenceException

[kalumpiwarra]

@asolntsev

That's exactly what I've requested. It also will make iterator behavior predictable, I mean no difference between collection access via iterator or index. Reload is the default behavior; accessing snapshot should be explicit.
I appreciate your efforts.

[bereg2k]

@asolntsev any chances to see this feature implemented any time soon? :)

[asolntsev]

@bereg2k Yes, I am always thinking about it :)
This feature is quite easy to do. But what I am really afraid is that it breaks backward compatibility and can make some tests much slower. That's why I am in doubt: should we make it configurable? Should we create another API for iterating collections?

Probably the ideal solution to me would be to change ElementsCollection so that it would not extend AbstractList. I mean, it should not have iterate methods at all. And we could have a separate method for converting it to List. Who needs a List, will need to call this method explicitly.

[bereg2k]

@asolntsev Regarding the backward compatibility: if you make ElementsCollection NOT to extend AbstractList this would definitely break some existing test code because obviously lots of folks iterate over ElementsCollection in their tests (I know, we do!).

Having said that, I like the idea of having a separate method for converting collections to List, something like $$.asList(). Seems like a pretty elegant solution.

[asolntsev]

@bereg2k Could you please review PR #1688 ?

Do these names sound reasonable: $$.asReloadingIterable() and $$.asFixedIterable()?

[bereg2k]

@asolntsev thank you for your effort!

I reviewed the PR, the method names sound descriptive enough.

Also, "fixed iterable" and "dynamic iterable" sound more like proper counterparts to each other.
At the same time, you can counterpart "reloading" with "non-reloading", but that's questionable...

[asolntsev]

Thank you for the review!
Yes, I like "fixed iterable" and "dynamic iterable".

[asolntsev]

Implemented in #1688