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

Add a shutdown_condition to setup #81

Closed
wants to merge 1 commit into from

Conversation

dnephin
Copy link

@dnephin dnephin commented Apr 7, 2015

Resolves #78

Add a shutdown_condition to setup() which can be used in environments that don't have a main thread with the expected name (or where you want to shutdown on some other condition).

@itamarst
Copy link
Owner

itamarst commented Apr 7, 2015

EventLoop is not in crochet.__all__, so I do not consider it a public API. Feel free to change it however you like.

@dnephin
Copy link
Author

dnephin commented Apr 7, 2015

Cool, updated so the tests should be passing now

@dnephin
Copy link
Author

dnephin commented Apr 7, 2015

Hmm, it looks like one test is failing under pypy, but I think it also failed on the last run of master as well: https://travis-ci.org/itamarst/crochet/builds/49051118

@itamarst
Copy link
Owner

itamarst commented Apr 7, 2015

Yeah, see #73 for issue about that.

@itamarst
Copy link
Owner

(Haven't had time to look at this in detail yet, but it's on my todo list).

_watchdog = Watchdog([t for t in threading.enumerate()
if t.name == "MainThread"][0], _registry.run)
register = _registry.register
def default_shutdown_condition():
Copy link
Owner

Choose a reason for hiding this comment

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

In retrospect it's not clear to me why this is module-level state at all. Seems like all of the state could live inside EventLoop, and then there's no need to pass a FunctionRegistry instance to EventLoop.__init__, it would just create one.

Copy link
Owner

Choose a reason for hiding this comment

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

I may be forgetting why this was necessary though, so if you think it's too big of a change for you (or this branch) it's not necessary to do this.

@itamarst
Copy link
Owner

Thanks for your contribution!

Besides the comments above:

  1. You're missing an end-to-end test of the new functionality, in some sense the most important test: "I can pass in a different function to setup() and crochet exits based on that function."
  2. Add a news entry, crediting yourself.
  3. Add a little documentation.

Or, if you feel like you've done enough I'd be happy to take over.

@dnephin
Copy link
Author

dnephin commented Apr 20, 2015

Cool, thanks for looking it over. I should be able to get back to this in the next few days and make those changes

@itamarst
Copy link
Owner

itamarst commented May 6, 2015

It looks like #83 actually fixed uWSGI. So maybe this is overkill lacking a known environment where it's necessary? I can certainly imagine such environments, mind you, just not sure if they exist in practice.

@dnephin
Copy link
Author

dnephin commented May 8, 2015

Yup, I'm happy with #83. I'd be fine closing this, or leaving it around in case it becomes necessary. Either works for me.

@itamarst
Copy link
Owner

I'll close, we can re-open if it turns out someone needs it.

@itamarst itamarst closed this May 12, 2015
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.

crochet doesn’t work with uWSGI
2 participants