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

PR for issue #674 #675

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open

PR for issue #674 #675

wants to merge 5 commits into from

Conversation

Darnethal0z
Copy link

installSignalHandlers parameter implementation on the reactor.run() method

@Darnethal0z Darnethal0z requested a review from a team as a code owner September 28, 2023 10:20
Copy link
Member

@glyph glyph left a comment

Choose a reason for hiding this comment

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

Hi, thanks for contributing to Klein!

I don't quite understand the desired effect of this change though, and the documentation seems inaccurate and confusing to me; it doesn't really explain why a user would or wouldn't want to pass this parameter.

This PR reads like something in your environment made you want the default behavior to be different, but I don't know what that thing was. Can you say more about why you wanted this in the first place?

Example -- Disabling signal handler installation
================================================

You have the possibility to print raised errors directly in the file descriptor specified in the ``Klein.run()`` ``logFile`` parameter, it can be useful for troubleshooting or logging fins.
Copy link
Member

Choose a reason for hiding this comment

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

There are reasons to want to have this parameter exposed, but … this is not one of them. Twisted logs raised errors automatically already, so this documentation just seems inaccurate? You also don't actually use the logFile parameter here, despite describing it. So this seems like an odd corner of the documentation.

It might be better to document how to call reactor.run or integrate Klein with twisted.internet.task.react directly, rather than copying every possible parameter into the run convenience function.

@Darnethal0z
Copy link
Author

Hi, thanks for the feedback.

The original idea was to call reactor.run in a custom way, i wanted to propose this because i got myself in a situation where i cannot find a way to catch raised exceptions from Twisted (i.e KeyboardInterrupt). It was more of a palliative solution that some peoples would find useful on their own.

It is apparently a lack of understanding of the Twisted documentation: After some experimentations i found a workaround on my side by combining reactor.addSystemEventTrigger and twisted.internet.task.react.


You mentionned that Klein could be integrated with the Twisted module twisted.internet.task.react.

Citing its description :

[...] Prefer this to calling reactor.run directly, as this function will also:

  • Take care to call reactor.stop once and only once, and at the right time.
  • Log any failures from the Deferred returned by main.
  • Exit the application when done, with exit code 0 in case of success and 1 in case of failure. If main fails with a SystemExit error, the code returned is used.

How do you think it can be done ? It may be unrelated to this PR.

@glyph
Copy link
Member

glyph commented Jun 25, 2024

It is apparently a lack of understanding of the Twisted documentation: After some experimentations i found a workaround on my side by combining reactor.addSystemEventTrigger and twisted.internet.task.react.

That's definitely the right way to handle this :)

@glyph
Copy link
Member

glyph commented Jun 25, 2024

How do you think it can be done ? It may be unrelated to this PR.

run() should be implemented in terms of react, I think, and expose only parameters interesting to Klein's customization. (It should probably also use an endpoint description rather than a "host" and "port" but that starts to get into just generally improving run overall)

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