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

allow disabling keep alive #646

Merged
merged 3 commits into from
Apr 24, 2017
Merged

allow disabling keep alive #646

merged 3 commits into from
Apr 24, 2017

Conversation

r0fls
Copy link
Contributor

@r0fls r0fls commented Apr 17, 2017

Fixes: #637

sanic/server.py Outdated

@property
def keep_alive(self):
return self.parser.should_keep_alive() and not self.signal.stopped
return (self.parser.should_keep_alive()
Copy link
Contributor

Choose a reason for hiding this comment

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

@property
def keep_alive(self):
    return (not self._no_keep_alive
            and not self.signal.stopped
            and self.parser.should_keep_alive())

Would be better?

@r0fls
Copy link
Contributor Author

r0fls commented Apr 17, 2017

Well this does get loaded from config here: https://github.com/channelcat/sanic/pull/646/files#diff-c4444a35dbd5e37ee480b0e8888e0880R682

How does #639 apply exactly?

@38elements
Copy link
Contributor

I am sorry.
I was misunderstanding for config.

@r0fls
Copy link
Contributor Author

r0fls commented Apr 17, 2017

Do we think the variable makes more sense as just KEEP_ALIVE vs NO_KEEP_ALIVE?

@38elements
Copy link
Contributor

38elements commented Apr 17, 2017

No, just I did not read config.py.
I think that NO_KEEP_ALIVE is good.

@38elements
Copy link
Contributor

About #639, it occurred when I implemented a similar test.

@38elements
Copy link
Contributor

I am sorry for changing my opinion.
Because I feel that KEEP_ALIVE is more common, I think that KEEP_ALIVE is better.

@r0fls
Copy link
Contributor Author

r0fls commented Apr 17, 2017

No worries, I think I agree so better late than never :) I'll update it to use that...

@38elements
Copy link
Contributor

Thanks.

@38elements
Copy link
Contributor

38elements commented Apr 17, 2017

I think that it is better to add tests for that keep alive is valid and invalid.
For #639, I think that the test when keep alive is valid will be difficult.

@38elements
Copy link
Contributor

38elements commented Apr 17, 2017

@r0fls
I understood the cause.
I commented in #639.
Would you implement this and add tests?

@38elements
Copy link
Contributor

My English is not good.
I am sorry if I confused you.

@r0fls
Copy link
Contributor Author

r0fls commented Apr 19, 2017

No you're English is fine. I'm just traveling. I'll try to add the test soon thank you. If you have an idea for it in mind you can put it here otherwise I'll plan to do it soon...

@38elements
Copy link
Contributor

Thanks.

@seemethere seemethere added this to the 0.5.2 milestone Apr 21, 2017
@r0fls r0fls merged commit b3814ca into sanic-org:master Apr 24, 2017
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.

None yet

4 participants