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

feat: add context support to router #285

Closed
wants to merge 1 commit into from

Conversation

johndeng
Copy link

@johndeng johndeng commented Dec 11, 2024

  • Add context parameter to Router.Route method
  • Add context parameter to router handlers
  • Update documentation and examples
  • Add context propagation tests

This change allows proper context handling in message routing, enabling timeout control and value propagation through the message handling chain.

- Add context parameter to Router.Route method
- Add context parameter to router handlers
- Update documentation and examples
- Add context propagation tests

This change allows proper context handling in message routing,
enabling timeout control and value propagation through the
message handling chain.
@johndeng johndeng closed this Dec 11, 2024
@johndeng johndeng reopened this Dec 11, 2024
@MattBrittan
Copy link
Contributor

Could you please raise an issue saying what issue you are trying to solve with this PR.

At first glance I'm not sure that this is a good idea for a couple of reasons (after a glance at the code - it's difficult to say much as I don't really know what problem you are trying to solve with this):

  • context.TODO() is a signal to the code author that action is needed, it has no real place in a library like this (it's fine while you are developing but I'd expect these to be removed before a PR is raised).
  • It would be expected that the context would be provided by the library, but it's unclear what should be used currently (you may plan to add more but it would be better to discuss this first)
  • generally handlers should execute rapidly and, in most cases, should not stop when the client is disconnecting (because this would lead to loss of messages). So if this change is to happen it will need clear documentation.

@johndeng
Copy link
Author

Sorry for any confusion this PR may have caused.

Let's move this discussion to #286 , I'll close this PR for now.

@johndeng johndeng closed this Dec 11, 2024
@MattBrittan
Copy link
Contributor

Thanks - and no worries. I'll add some comments on the issue you raised, I'm open to changes to the library but as it's difficult to remove a feature once it's added some care is needed.

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