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(#170): wait for the IOLoop to stop before attempting to close it #276

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

kkostov
Copy link
Contributor

@kkostov kkostov commented Jun 29, 2024

Hello!

Thank you for this amazing library 🤩 !

It works really well when watching static assets and templates, however I noticed that changes to .py files fails due to a runtime error. This issue has been previously recorded in #170, and marked as "needs more info". In my Flask project, I'm able to reproduce it continuously on every change to a .py file in the project.

After following the comments on the ticket, I realized the technique to restart tornado can be improved in order to avoid the runtime error. This PR introduces a slight change in the order of disposing of resources:

From the docs

  • The call to IOLoop.instance().start() does not return until the program calls IOLoop.instance().stop().
  • The call to IOLoop.instance().stop() returns immediately, however it takes some time for the loop to be shutdown. A complete stop is fact only when the call to IOLoop.instance().start() returns.

So to avoid the crash, the call to .close() must be after the call to .start() has returned.

 # When autoreload is triggered, initiate a shutdown of the IOLoop
add_reload_hook(lambda: IOLoop.instance().stop())
# The call to start() does not return until the IOLoop is stopped.
IOLoop.instance().start()
# Once the IOLoop is stopped, the IOLoop can be closed to free resources
IOLoop.current().close(all_fds=True)

ℹ️ The tornado docs include an additional recommendation on how to handle this scenario differently (without having to reboot the loop) but that may have other unintended side effects, so I didn't consider it further.

ℹ️ My setup is as follows:

  • macOS 14.5
  • Python 3.12.3
  • requirements:
Jinja2==3.1.4
livereload==2.7.0
tornado==6.4.1
webassets==2.0
Flask==3.0.3

@davidparsson
Copy link

@lepture, any chanses of getting this merged and released anytime soon? There seem to be many of us that are affected by the problem that this resolves.

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.

3 participants