Skip to content

Conversation

cubicdaiya
Copy link
Contributor

I felt like this feature on seeing #334.

@agentzh
Copy link
Member

agentzh commented Feb 12, 2014

@cubicdaiya Thank you for the patch! It looks generally good in a quick peek. I'll try to review and merge this shortly.

@agentzh
Copy link
Member

agentzh commented Feb 19, 2014

@cubicdaiya Sorry for the delay on my side! I've just reviewed your patch and made some changes:

  1. It is better to allocate the lowcase_key buffer in the Lua GC to prevent in-request memory leaks in the NGINX request pools.
  2. We should skip ngx_table_elt_t entries in r->headers_out.headers whose hash fields are 0. (I've also added a test case for this.)

Will you review and test my modified version of your patch below?

http://openresty.org/download/ngx-lua-resp_get_headers.patch

Thanks!

@cubicdaiya
Copy link
Contributor Author

@agentzh Thank you for review and modification.

After your patch is applied, It seems to work fine in my environment.

@agentzh
Copy link
Member

agentzh commented Feb 19, 2014

@cubicdaiya I'm also thinking about sharing the metatable between ngx.req.get_headers and ngx.resp.get_headers. What do you think?

@cubicdaiya
Copy link
Contributor Author

@agentzh I think it's better. I have applied your patch and the changes for the above-described requirement. What do you think about my changes?

@agentzh
Copy link
Member

agentzh commented Feb 20, 2014

@cubicdaiya Sorry I'm very busy with other things these days. I'll check out your latest changes in the next week :) Thanks!

agentzh added a commit that referenced this pull request Feb 26, 2014
…ll the response headers. thanks Tatsuhiko Kubo for the patch in #335.
@agentzh
Copy link
Member

agentzh commented Feb 26, 2014

@cubicdaiya Already applied a slightly modified version of your patch to master.

Basically I removed the line-trailing spaces and redundant blank lines. I also fixed source lines exceeding 80 columns in your patch.

Thank you for your contribution!

@agentzh agentzh closed this Feb 26, 2014
@cubicdaiya cubicdaiya deleted the feature/ngx_resp_get_headers branch March 1, 2014 07:35
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