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.

Sign up for GitHub

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

perf(response-transformer) accumulate chunks of response using table #2977

Merged
merged 4 commits into from
Oct 29, 2017
Merged

perf(response-transformer) accumulate chunks of response using table #2977

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
db71661
perf(response-transformer) accumulate chunks of response using table
Oct 22, 2017
c29eaf1
perf(response-transformer) accumulate chunks of response using table
Oct 25, 2017
fcb1a22
perf(response-transformer) accumulate chunks of response using table
Oct 26, 2017
0e41f43
minor tweak
Tieske Oct 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions kong/plugins/response-transformer/handler.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ end

function ResponseTransformerHandler:access(conf)
ResponseTransformerHandler.super.access(self)
ngx.ctx.buffer = ""
ngx.ctx.chunks = {}
ngx.ctx.chunk_number = 1
Copy link
Member

Choose a reason for hiding this comment

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

though it also applied on the original code, my comment would be to use a better namespaced name for these variables. The ngx.ctx table is shared for the lifetime of the request and hence has a risk of name colissions, especially with very generic names as chunks.

So maybe ngx.ctx.rt_body_chunks ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

Copy link
Contributor Author

@r-alekseev r-alekseev Oct 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

By the way, perhaps, better kind of namespacing is to create a separate table for each plugin.
f.e. ctx.response_transformer = { body_chunks = {}, body_chunk_number = 1 }
But this is breaking change and need to be tested hard if plugins get access to shared data.

end

function ResponseTransformerHandler:header_filter(conf)
Expand All @@ -28,10 +29,11 @@ function ResponseTransformerHandler:body_filter(conf)
if is_body_transform_set(conf) and is_json_body(ngx.header["content-type"]) then
local chunk, eof = ngx.arg[1], ngx.arg[2]
if eof then
local body = body_filter.transform_json_body(conf, ngx.ctx.buffer)
local body = body_filter.transform_json_body(conf, table.concat(ngx.ctx.chunks))
ngx.arg[1] = body
else
ngx.ctx.buffer = ngx.ctx.buffer .. chunk
ngx.ctx.chunks[ngx.ctx.chunk_number] = chunk
ngx.ctx.chunk_number = ngx.ctx.chunk_number + 1
Copy link
Member

Choose a reason for hiding this comment

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

in this code there are a lot of ngx.ctx accesses. Since the lookup of ctx in ngx is rather expensive (it has some meta table magic), it would be better to cache it in a local.

-- at the top
local ctx = ngx.ctx`

and then replace all ngx.ctx.whatever to ctx.whatever

Copy link
Contributor Author

@r-alekseev r-alekseev Oct 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

context cached in local variables in both ('access' and 'body_filter') functions

ngx.arg[1] = nil
end
end
Expand Down