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

Implement ngx.get_phase() #78

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@p0pr0ck5
Contributor

p0pr0ck5 commented Jan 5, 2017

No description provided.

@p0pr0ck5

This comment has been minimized.

Contributor

p0pr0ck5 commented Jan 5, 2017

@p0pr0ck5

This comment has been minimized.

Contributor

p0pr0ck5 commented Jan 5, 2017

Not sure what's going on with Travis here, seems odd that CPAN fails to install Crypt-SSLeay:

! Configure failed for Crypt-SSLeay-0.72. See /home/travis/.cpanm/work/1483587597.7242/build.log for details.

Apparently manually installing libssl-dev manually wasn't the fix :p

}
local errmsg = base.get_errmsg_ptr()

This comment has been minimized.

@agentzh

agentzh Jan 5, 2017

Member

I'm afraid we use two blank lines to separate different kinds of global constructs in this library consistently.

This comment has been minimized.

@agentzh

agentzh Jan 5, 2017

Member

Please see other lib/resty/core/*.lua files for reference.

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 5, 2017

Contributor

Whups. Will fix up. Thanks!

return "ssl_cert"
elseif context == 0x0800 then
return "ssl_session_store"
elseif context == 0x1000 then

This comment has been minimized.

@agentzh

agentzh Jan 5, 2017

Member
  1. We should give these magic numbers a name if we want to do this with cascaded if/elseif.
  2. It's better to use a Lua table to speed up look up instead of the series of cascaded if and elseif.

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 5, 2017

Contributor

Wasn't sure which style (case-switch or table lookup) was the better choice here. Let me know what you think of the next patchset!

@@ -0,0 +1,273 @@
# vim:set ft= ts=4 sw=4 et fdm=marker:

This comment has been minimized.

@agentzh

agentzh Jan 5, 2017

Member

The test cases in this library for the FFI-version of ngx_lua's Lua API should focus instead of JIT-ability instead of functionality. Functionality tests are already in the ngx_lua module and ngx_lua's test suite can run with lua-resty-core with the following environment setting, for example:

export TEST_NGINX_INIT_BY_LUA="package.path = (package.path or '') .. ';$PWD/../lua-resty-core/lib/?.lua;$PWD/../lua-resty-lrucache/lib/?.lua;' require 'resty.core'"

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 5, 2017

Contributor

The tests here (with a few exceptions where I ran into some Perl parsing issues in tests 8 and 9, I'll take a deeper look at those) do test JIT-ability, copying the same logic with regard to examining the output of jit.v (I slimmed down the init_by_lua_block definition since jit.dump never actually runs, but I can add it back for completeness if needed). I also wanted to test the response of the new get_phase() implementation to ensure that we're handing back the same results as we expect from the C API, but if this is unnecessary I will remove it.

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 5, 2017

Contributor

(also if you have any idea of why Travis was/is failing installing Test::Nginx via cpanm, lemme know)

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 5, 2017

Contributor

(although i hindsight, i understand what you mean about testing the functionality of the results with the lua-nginx-module tests and loading in resty.core; ill slim down these tests to just examing the JIT-ability; sorry for the confusion!)

Use a lookup table for phase translation
Also contains some style/newline fixes
local errmsg = base.get_errmsg_ptr()
local context_lookup = {

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 5, 2017

Contributor

There is also this form we could use:

local context_lookup = {}
table.insert(context_lookup, 0x0001, "set")
table.insert(context_lookup, 0x0002, "rewrite")
table.insert(context_lookup, 0x0004, "access")
table.insert(context_lookup, 0x0008, "content")
table.insert(context_lookup, 0x0010, "log")
table.insert(context_lookup, 0x0020, "header_filter")
table.insert(context_lookup, 0x0040, "body_filter")
table.insert(context_lookup, 0x0080, "timer")
table.insert(context_lookup, 0x0100, "init_worker")
table.insert(context_lookup, 0x0200, "balancer")
table.insert(context_lookup, 0x0400, "ssl_cert")
table.insert(context_lookup, 0x0800, "ssl_session_store")
table.insert(context_lookup, 0x1000, "ssl_session_fetch")

This prevents us from needing to call tostring() below, I'm just not sure how we feel about arrays with empty indices. It also performs 3 times faster in the following micro benchmark:

require "resty.core"
local phase
for i = 1, 1000000 do
    phase = ngx.get_phase()
end

The form in the existing PR (string converted lookup) takes about 30000 microseconds to run; the form in this comment (integer lookup) takes about 10000 microseconds to run. Let me know what you think the best approach is here, if we need to be worried about this much micro performance analysis in this context (if this even is a valid micro benchmark, or I missed something), or if we should focus just on coding style/readability. Thanks!

This comment has been minimized.

@agentzh

agentzh Jan 6, 2017

Member

@p0pr0ck5 You know that Lua table keys can be arbitrary numbers instead of strings?

local errmsg = base.get_errmsg_ptr()
local context_lookup = {

This comment has been minimized.

@agentzh

agentzh Jan 6, 2017

Member

@p0pr0ck5 You know that Lua table keys can be arbitrary numbers instead of strings?

["512"] = "balancer",
["1024"] = "ssl_cert",
["2048"] = "ssl_session_store",
["4096"] = "ssl_session_fetch",

This comment has been minimized.

@agentzh

agentzh Jan 6, 2017

Member

Why use strings as the keys here? You should use numbers directly. Lua tables should be clever enough to use a hash table instead of a sparse array for this table.

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 10, 2017

Contributor

Fixed up! Let me know if this approach needs further adjustment.

error(errmsg)
end
local ctx_str = tostring(context)

This comment has been minimized.

@agentzh

agentzh Jan 6, 2017

Member

This tostring() call is very expensive since it creates a new Lua string which is a GC object. You should use the numeric value directly (but remember to use tonumber() instead of the raw cdata object).

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 10, 2017

Contributor

The value returned from the C function is of type int, so this should return a Lua number type, right? I can wrap this return value in tonumber() for style/clarity, but it seems like an unnecessary function call.

function ngx.get_phase()
local r = getfenv(0).__ngx_req
-- If we have no request object, assume we are called from the "init" phase

This comment has been minimized.

@agentzh

agentzh Jan 6, 2017

Member

Style: we deliberately avoid using upper case letters in such code comment at the beginning of the sentence.

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 10, 2017

Contributor

Bah, copied directly from the associated C function. my bad :)

end
local context = C.ngx_http_lua_ffi_get_phase(r, errmsg)
if context == -1 then -- NGX_ERROR

This comment has been minimized.

@agentzh

agentzh Jan 6, 2017

Member

We should use FFI_ERROR instead of the magic number here. See other similar places in this library.

This comment has been minimized.

@p0pr0ck5
@agentzh

This comment has been minimized.

Member

agentzh commented Jan 6, 2017

@p0pr0ck5 Thanks for your contribution! Please see my latest comments :)

Address reviewer comments
Context lookup table is now a sparse, integer-indexed table,
obviating several string operations. Additionally, the base FFI_ERROR
definition is used to check for NGX_ERROR, and tests have been
slimmed down to explicitly check for the JIT-ability of this module.
@p0pr0ck5

This comment has been minimized.

Contributor

p0pr0ck5 commented Jan 10, 2017

@agentzh thanks for the feedback! I need to stop writing code when supposed to leave for the airport in 30 minutes ;) new patchset is up, let me know what you think.

local FFI_ERROR = base.FFI_ERROR
local tostring = tostring
ffi.cdef[[

This comment has been minimized.

@thibaultcha

thibaultcha Jan 10, 2017

Member

style: I believe we are missing a line jump before this call (2 blank lines)

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 10, 2017

Contributor

idem

@@ -0,0 +1,55 @@
local ffi = require 'ffi'
local C = ffi.C
local base = require "resty.core.base"

This comment has been minimized.

@thibaultcha

thibaultcha Jan 10, 2017

Member

I believe this library consistently groups requires and caching, and separates them by 2 blank lines. It also uses double quotes:

local ffi = require "ffi"
local base = require "resty.core.base"


local C = ffi.C
local tostring = tostring
local FFI_ERROR = base.FFI_ERROR

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 10, 2017

Contributor

There are a handful of modules that do not adhere to this standard; trying to figure out which style is appropriate to copy is... nebulous and tricky. :) I don't at all mind keeping consistent style (rather glad to see it noted and enforced), but having some dev docs about this kind of thing would be great :D

For reference:

https://github.com/openresty/lua-resty-core/blob/master/lib/resty/core/uri.lua#L4 - all requires and redefs without newline separation
https://github.com/openresty/lua-resty-core/blob/master/lib/resty/core/hash.lua#L4 - all requires and redefs without newline separation
https://github.com/openresty/lua-resty-core/blob/master/lib/resty/core/exit.lua#L4- all requires and redefs without newline separation

Again: I'm very glad to have a consistent style. And I'd be very happy to put up a PR to fix existing modules that do not adhere to said style. But having it documented so devs have a point of reference aside from guessing from existing context would be awesome.

local _M = {
_VERSION = base.version
}

This comment has been minimized.

@thibaultcha

thibaultcha Jan 10, 2017

Member

idem (2 blank lines)

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 10, 2017

Contributor

idem

context_lookup[0x0200] = "balancer"
context_lookup[0x0400] = "ssl_cert"
context_lookup[0x0800] = "ssl_session_store"
context_lookup[0x1000] = "ssl_session_fetch"

This comment has been minimized.

@thibaultcha

thibaultcha Jan 10, 2017

Member

I am curious as to why this form is being used to assign values to this table? Isn't this triggering 4 rehashes of the table? Additionally, isn't it messier than simply:

local context_lookup = {
    [0x0001] = "set",
    ...
}

And last of all, I am unsure about this but shouldn't the name be capitalized, as this is meant to be a constant? I am not familiar with the preferred naming style with such "constant tables".

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 10, 2017

Contributor

silly mistake by me. i didnt realize that brackets could be used to defined keys as integers, in addition to strings. my fault for not understanding the syntax enough here :)

as for re-hashes, i believe this occurs when the table is require'd the first time; i dont know if it happens on subsequent requires (given the context of lua-resty-core, this seems unlikely), so when taking this approach i weighed against pcalling table.new (since it would only be called once, a few re-hashes isnt a big deal, and i didnt want to bikeshed).

as for capitalization, im not sure, non-table constants are indeed all-caps, not sure if this applies to a table such as this. ill happily adjust as needed once a definitive, consistent style is declared.

This comment has been minimized.

@thibaultcha

thibaultcha Jan 10, 2017

Member

The rehash is just a nitpick since this is indeed only executed upon the initial loading of the module. Still a valid argument and useful for future reference I'd say, hence the mention :)

local _M = {
_VERSION = base.version

This comment has been minimized.

@thibaultcha

thibaultcha Jan 10, 2017

Member

It seems other modules in this library export the version number in a version field (non-capitalized, no _ prefix). They also avoid creating a _M module table if not necessary. Shouldn't we try staying consistent?

Example: https://github.com/openresty/lua-resty-core/blob/master/lib/resty/core/time.lua

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 10, 2017

Contributor

It seems there are multiple styles at play here in this lib (at least 3 that i can find). See my comment above wrt. uri/hash/exit which seem to exhibit an earlier style. Additionaly, some of the ngx modules exhibit conflicting style wrt. cdef indentation. Again, documented style would be very useful for situations like there.

local tostring = tostring
ffi.cdef[[
int ngx_http_lua_ffi_get_phase(ngx_http_request_t *r, char **err)

This comment has been minimized.

@thibaultcha

thibaultcha Jan 10, 2017

Member

style: I believe this is missing identation

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 10, 2017

Contributor

Similar to other comments wrt. multiple style in multiple modules in the project, I can't seem to find a consistent style, even in regard to ngx vs resty modules; in both cases, some modules exhibit one style, some modules exhibit another.

local context = C.ngx_http_lua_ffi_get_phase(r, errmsg)
if context == FFI_ERROR then -- NGX_ERROR
error(errmsg)

This comment has been minimized.

@thibaultcha

thibaultcha Jan 10, 2017

Member

All occurrences of error() in this library use the return error() form (which has a similar behavior as error("", 2)) and thus matters in our case. The same apply for the later call to error() in this function.

This comment has been minimized.

@p0pr0ck5

p0pr0ck5 Jan 10, 2017

Contributor

Thanks! Fixed.

@thibaultcha

This comment has been minimized.

Member

thibaultcha commented Jan 10, 2017

As @p0pr0ck5 pointed out to me regarding style comments (modules loading/caching and FFI cdef indentation), it appears some modules use a slightly different style, or are less pedantic (possibly because they are older?). Example: https://github.com/openresty/lua-resty-core/blob/master/lib/resty/core/exit.lua#L4-

If so, and if the style described in my comments (which is already in use in other modules) is correct, it may be worth it to update those modules for the sake of consistency?

Style updates
Bring some sanity (and capitalization) to the context lookup table.
Also attempts to bring this module into style alignment wrt. other
modules in this library.
@p0pr0ck5

This comment has been minimized.

Contributor

p0pr0ck5 commented Jan 10, 2017

Thanks @thibaultcha for the comments. I put up another patchset; it seems there are some style inconsistency issues that need to be looked at.

Again, to re-iterate: I'll be happy to conform to whatever style is appropriate for this lib. We just need to figure out what that style is and define/enforce it consistently. :)

@p0pr0ck5

This comment has been minimized.

Contributor

p0pr0ck5 commented Oct 15, 2017

@agentzh any thoughts here?

@@ -0,0 +1,57 @@
local ffi = require 'ffi'
local base = require "resty.core.base"

This comment has been minimized.

@agentzh

agentzh Oct 16, 2017

Member

Style: 2 blank lines needed here.

@agentzh

This comment has been minimized.

Member

agentzh commented Nov 11, 2017

@p0pr0ck5 Will you rebase to the latest master and resolve the merge conflicts please? Then I'll have another look. Thanks!

@@ -258,7 +258,6 @@ TODO
====
* Re-implement `ngx_lua`'s cosocket API with FFI.
* Re-implement `ngx_lua`'s `ngx.get_phase` API function with FFI.
* Re-implement `ngx_lua`'s `ngx.eof` and `ngx.flush` API functions with FFI.

This comment has been minimized.

@agentzh

agentzh Jan 2, 2018

Member

We should also add a doc section for the "resty.core.phase" module here.

my $pwd = cwd();
our $HttpConfig = <<_EOC_;
init_by_lua '

This comment has been minimized.

@agentzh

agentzh Jan 2, 2018

Member

Newly added tests should always use the *_by_lua_block {} directives.

Also, we need to add the Lua module search paths correctly, just like the existing .t files, like this:

lua_package_path "$pwd/lib/?.lua;../lua-resty-lrucache/lib/?.lua;;";
local context = C.ngx_http_lua_ffi_get_phase(r, errmsg)
if context == FFI_ERROR then -- NGX_ERROR
return error(errmsg)

This comment has been minimized.

@agentzh

agentzh Jan 2, 2018

Member

Better avoid tail calls so that we can have complete Lua backtrace when this error happens. And yeah, the existing code base of this library may need similar fixes as well.

local phase = CONTEXT_LOOKUP[context]
if not phase then
return error("unknown phase: " .. tostring(context))

This comment has been minimized.

@agentzh

agentzh Jan 2, 2018

Member

Ditto.

local errmsg = base.get_errmsg_ptr()
local CONTEXT_LOOKUP = {

This comment has been minimized.

@agentzh

agentzh Jan 2, 2018

Member

I think context_names is a better variable name here. We only use all-upper-case form for scalar constant variable names.

@agentzh

This comment has been minimized.

Member

agentzh commented Jan 2, 2018

I've fixed all the aforementioned minor issues and resolved the conflicts while rebasing to the latest master. Merged. Thanks!

@agentzh agentzh closed this Jan 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment