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

Fixing #779 by settings http protocol and allowing to set keep alive … #780

Merged
merged 3 commits into from Dec 17, 2019

Conversation

bloodbare
Copy link
Member

…timeout

@@ -91,5 +91,7 @@
"search_parser": "default",
"object_reader": "guillotina.db.reader.reader",
"thread_pool_workers": 32,
"keep_alive": 5,
Copy link
Member

Choose a reason for hiding this comment

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

5 is very low and I'm not sure it'll actually help the situation if this is the same problem that I've seen.

fwiw, nginx defaults to 75sec keep alive I believe.

Copy link
Member Author

Choose a reason for hiding this comment

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

default keep alive timeout at uvicorn is 5 (https://github.com/encode/uvicorn/blob/master/uvicorn/config.py#L134)

@@ -91,5 +91,7 @@
"search_parser": "default",
"object_reader": "guillotina.db.reader.reader",
"thread_pool_workers": 32,
"keep_alive": 5,
"http_protocol": "h11"
Copy link
Member

Choose a reason for hiding this comment

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

it's a shame we end up with h11 here

Copy link
Member Author

Choose a reason for hiding this comment

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

Its temporal until encode/uvicorn#344 is fixed

@masipcat
Copy link
Contributor

masipcat commented Dec 17, 2019

I'm not sure if we want to expose all these options in the cli / app_settings in the top level. I'd suggest something like:

app_settings = {
    "server_settings": {
         "uvicorn": { ... }
     }
}

that is mapped to the uvicorn Config (pseudo-code):

uvicorn.Config(**app_settings["server_settings"]["uvicorn"])

@bloodbare bloodbare merged commit 65a06d7 into master Dec 17, 2019
@bloodbare bloodbare deleted the http11 branch December 17, 2019 16:06
@codecov-io
Copy link

codecov-io commented Dec 17, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@ee8dbe3). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #780   +/-   ##
========================================
  Coverage          ?     94%           
========================================
  Files             ?     308           
  Lines             ?   27619           
  Branches          ?       0           
========================================
  Hits              ?   25937           
  Misses            ?    1682           
  Partials          ?       0
Impacted Files Coverage Δ
guillotina/_settings.py 100% <ø> (ø)
guillotina/contrib/dbusers/services/roles.py 77.8% <ø> (ø)
guillotina/contrib/dbusers/services/__init__.py 100% <100%> (ø)

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