Fix of #122 #123
Fix of #122 #123
Conversation
bjoern/request.c
Outdated
| } | ||
| } | ||
| /* SERVER_NAME is required */ | ||
| else { |
There was a problem hiding this comment.
In theory neither SERVER_NAME nor SERVER_PORT must be empty. Should a sane default of "localhost" be used? HTTP_HOST should be preferred by the app. With unix sockets, should the port be set to 80? Url reconstruction will be wrong with unix sockets in any way!
There was a problem hiding this comment.
No, let's leave them empty. This is a case not covered by PEP 333, and we better break applications completely rather than sneaking in wrong values that cause hard-to-debug issues.
bjoern/request.c
Outdated
| @@ -205,7 +205,18 @@ on_message_complete(http_parser* parser) | |||
| /* SERVER_NAME and SERVER_PORT */ | |||
| if (REQUEST->server_info->host) { | |||
| _set_header(_SERVER_NAME, REQUEST->server_info->host); | |||
| _set_header(_SERVER_PORT, REQUEST->server_info->port); | |||
| if (REQUEST->server_info->port == Py_None) { | |||
| _set_header(_SERVER_PORT, | |||
There was a problem hiding this comment.
I guess these environment items could be moved to _initialize_request_module as they never change? Also, please remove these line breaks.
There was a problem hiding this comment.
They could be set in theory. But _initialize_request_module does not have "access" to the real values as the host name or port number can differ in different setups.
As for the line breaks: I guess you mean I should write
_set_header(_SERVER_PORT, PyUnicode_FromFormat("%i", REQUEST->server_info->port));instead of
_set_header(_SERVER_PORT,
PyUnicode_FromFormat("%i", REQUEST->server_info->port));shouldn't I?
There was a problem hiding this comment.
But _initialize_request_module does not have "access" to the real values as the host name or port number can differ in different setups.
You can simply pass &info to that function (see _bjoernmodule.c). They won't change while the server is running, or am I missing something?
As for the line breaks
Yes that's what I meant. 90-100 chars line width is fine.
There was a problem hiding this comment.
You your are right, that would be possible.
|
@jonashaag So is c894bb4 what you meant? |
I just moved them. |
As said, the pull request.
If
SERVER_NAMEorSERVER_PORTare not, an empty string is set instead.