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 long_callback for slow responses and small polling intervals #2058

Closed
wants to merge 1 commit into from

Conversation

sebbegg
Copy link

@sebbegg sebbegg commented May 18, 2022

This fixes (#1769)

This avoids the race-condition between the long_callback interval triggers and the long callback returning slow/large payloads.
As mentioned in #1769 this PR basically does the following:

  • Instead of returning the result of a long callback directly from the interval callback, the interval callback only signals that the result is ready by setting the result key and job id on an newly added "result" helper store
  • When the result is ready, the interval is disabled directly (not delayed to the next interval trigger) and the user store data is cleared
  • A newly added callback on the "result" store fetches the result

The code snippets for reseting the user store seem a little redundant between the should_cancel branch and the new parts of code. I'd be happy to refactor that somewhat, but wanted to keep the diff as small as possible for a start.

Contributor Checklist

  • I have run the tests pytest -k long_callback locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follows
    • this GitHub #PR number updates the dash docs
    • here is the show and tell thread in Plotly Dash community

Sebastian Eckweiler (sebastian.eckweiler@mercedes-benz.com), Mercedes-Benz AG on behalf of MBition GmbH (Imprint)

@sebbegg sebbegg changed the title Add result fetching callback to long_callback Fix long_callback race-condition for slow responses and small polling intervals May 18, 2022
@sebbegg sebbegg changed the title Fix long_callback race-condition for slow responses and small polling intervals Fix long_callback for slow responses and small polling intervals May 19, 2022
@T4rk1n
Copy link
Contributor

T4rk1n commented May 20, 2022

This fix is only partial, works ok for larger response. But when I tested this with the first app example from #1769, I still get infinite loops and the callback doesn't resolve and I can't cancel.

I think as long as the long callbacks requests are made via an interval component, there is bound to have issues with the polling. There is also issues with the extra components not playing well with the coming up multi page feature.

As such, we have decided to improve the long callbacks and bring them on par with regular callbacks. I am refactoring the long callback to be handled in the renderer with a request polling chain to replace the interval and extra components added by the current long callback solution. There will only be one request at time, when the fetch completes, it will either have the progress updates and restart the fetch after a timeout, or complete the initial callback outputs.

Thank you for your contribution, really helped me have a better grasp of the timing issues. I will close this PR as it's not completely fixed and would require more work while I am currently working on an overhaul of the long callbacks.

@T4rk1n T4rk1n closed this May 20, 2022
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.

2 participants