pollInterval is broken, skips every other interval #4786
Comments
benjamn
added a commit
that referenced
this issue
May 10, 2019
The last time I touched this code in #4243, I thought it was worthwhile to preserve the behavior of sometimes batching polling query fetches in the same tick of the event loop. However, as @voxtex points out in #4786, almost any batching strategy will lead to unpredictable fetch timing, skipped fetches, poor accounting for fetch duration (which could exceed the polling interval), and so on. On top of that, even the premise that "batching is good" comes into question if it doesn't happen consistently. An implementation which uses independent timers seems much simpler and more predictable.
benjamn
added a commit
that referenced
this issue
May 10, 2019
The last time I touched this code in #4243, I thought it was worthwhile to preserve the behavior of sometimes batching polling query fetches in the same tick of the event loop. However, as @voxtex points out in #4786, almost any batching strategy will lead to unpredictable fetch timing, skipped fetches, poor accounting for fetch duration (which could exceed the polling interval), and so on. On top of that, even the premise that "batching is good" comes into question if it doesn't happen consistently. An implementation which uses independent timers seems much simpler and more predictable.
|
Just wanted to say thanks for working on this, very helpful! |
|
Closing since we just published the final version of |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Intended outcome:
Query pollInterval should refresh data every x ms.
Actual outcome:
Every other interval is skipped due to a bug in the polling logic.
How to reproduce the issue:
https://codesandbox.io/s/9jwwplw56y
Versions
2.5.1
pollInterval is broken as demonstrated by the example above (open up your network console). A 5 second pollInterval is only polling every 10 seconds due to the way it is implemented.
Because lastPollTime is updated asynchronously after the query completes while the setTimeout is queued up immediately, lastPollTime will always be
now() - (timeLimitMs + fetchDuration)which will never be <=(now - timeLimitMs)on the subsequent timeout. It will always skip the immediately following timeout execution and only pass the condition on the next one.https://github.com/apollographql/apollo-client/blob/master/packages/apollo-client/src/core/QueryManager.ts#L1604
For a concrete example, imagine pollInterval = 5s, a query takes 1s to execute, and now = 0. We execute the query, queue up a timeout execution 5s from now (5s), and then the query comes back and we update lastPollTime (1s). So on timeout execution
(now - lastPollTime) >= intervalis(5 - 1) >= 5which is false and we have to wait for another timeout execution before anything will happen.Finally, is the batched polling functionality documented anywhere? I feel like this needs to be documented and also have an option to opt out. If I have 2 queries on the page with separate intervals of 9s and 10s, it will take 18s (!) for the 10s poll interval to actually execute. This does not seem like a good default behavior and was completely unexpected when I was working with the client.
The text was updated successfully, but these errors were encountered: