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

Support for custom headers #24

Closed
wants to merge 2 commits into from
Closed

Conversation

edubart
Copy link

@edubart edubart commented Jun 5, 2017

For example can pass custom headers (in this case the Cookie) in the client websocket by doing
wb:connect(uri, {headers = { Cookie = 'mycookie' })

Note that with this option the options origin and protocols could be removed and , then the header generation code simplified, however I did not want to break API compability.

This implements #23

Copy link
Member

@agentzh agentzh left a comment

Choose a reason for hiding this comment

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

I think we also need some corresponding test cases for this new feature in the existing test suite.

Need some docs as well.

Thanks!

if headers then
custom_header = ""
for field,value in pairs(headers) do
custom_header = custom_header .. "\r\n" .. field .. ": " .. value
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 very bad to use .. in a loop. Lua strings are immutable and this can easily lead to O(n^2) time complexity and O(n^2) space complexity. Better use a globally shared Lua table to collect all these string pieces.

Copy link
Author

@edubart edubart Jun 6, 2017

Choose a reason for hiding this comment

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

Indeed, however is not like this is a critical code, neither many of custom headers usually will be used and I see no guaranteed performant way to avoid concacenating with ".." while maintaining the API the design.

We could use string.format() in each iteration here, but is not clear if it would perform better. We could also create an auxiliary table and use table.concat(), but again is not clear if creating another table (with strings inside which would also so be collected) to do the task would perform better.

Anyway I have no intention to spend time micro optimazing here as this is not a critical code.

Copy link
Member

Choose a reason for hiding this comment

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

@edubart This table can be shared by all the function invocations. You do not even need to clear the table actually.

Copy link
Member

Choose a reason for hiding this comment

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

@edubart In fact, this piece of code is run in a request handler. I'd say code run by every request should be considered "hot code path".

Copy link
Author

Choose a reason for hiding this comment

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

I have no intention to do anything futher, I will leave using my fork as in my case the performance difference is negligible.

local headers = opts.headers
if headers then
custom_header = ""
for field,value in pairs(headers) do
Copy link
Member

Choose a reason for hiding this comment

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

Style: need a space after the comma (,).

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's better to use a flat array for this option, as in

{ name1, value1, name2, value2, ...}.

So that we can avoid iterating through a hash table. Additionally, iterating through a hash table is not JIT compiled in LuaJIT.

Copy link
Author

@edubart edubart Jun 6, 2017

Choose a reason for hiding this comment

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

Although this is not the most performant way it maintains the usual design of other lua libraries APIs, like lua-resty-http does for headers, which I also use in resty projects, and different APIs design across libraries usually is undesired, thus I think using hash table instead of flat arrays is for the better.

Copy link
Member

@agentzh agentzh Jun 6, 2017

Choose a reason for hiding this comment

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

@edubart The lua-resty-http library you referenced is not our work, so we do not really care about being consistent with all the 3rd-party libraries. That's not even possible anyway. The upcoming ngx.req.get_header_list() and ngx.resp.get_header_list() API functions of the OpenResty core will use a similarly structured flat array. OpenResty's core APIs and libraries care more about performance.

@agentzh
Copy link
Member

agentzh commented Jun 5, 2017

@edubart Maybe it's easier to just add support for a cookie option? That would also be more efficient.

@edubart
Copy link
Author

edubart commented Jun 6, 2017

@agentzh Just adding cookie is not sufficient, I also need to use other header options like Authentication, and some custom ones like Api-Key, this way is more flexible, another users may need different headers aswell.

@edubart
Copy link
Author

edubart commented Jun 6, 2017

A test and documentation were added.

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

2 participants