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

Remove context error check in lifecycle #1034

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

yux0
Copy link

@yux0 yux0 commented Feb 7, 2023

The context canceled or deadline exceeded errors should be handled by user hooks.

E.g.

  1. Hook 1 started.
  2. Hook 2 not started due to context expired.

Hook 1 did not stop because of context expired.

Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @yux0 ,

Thank you for taking the time to contribute but please follow our guidelines for contributing. We do request issues are filed to describe the change you are contributing to ensure the maintainers are aligned with the changes.

Unfortunately, this change is rather something we would not do because it is an explicit design choice. Not all user hooks provided to fx respect context, so the framework can't assume that they do.

On that note, there is an issue that spawned up an internal design note on how we can support different implementations of lifecycles - ex. customizable lifecycle for Fx so that you can implement things such as this. I will share that in the form of GitHub issue shortly and link this PR there too, so you can follow along there once that is posted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants