ngx.process.master_pid by chronolaw · Pull Request #158 · openresty/lua-resty-core

chronolaw · 2017-12-28T04:58:20Z

Here is the new API ngx.process.master_pid, which needs ffi C function ngx_http_lua_ffi_master_pid in ngx_lua.

I also write a test case in t/process-master-pid.t, But I am not familar with Perl and have not found the detail API documents of test::nginx, so I just copied worker.t then modified on it.

That test case may has error, please help me to correct it. Thanks very much.

merge from latest openresty

membphis · 2017-12-29T08:30:08Z

you need to change the travis config, use your own branch to test.

https://github.com/openresty/lua-resty-core/blob/master/.travis.yml#L62

agentzh · 2018-01-02T00:10:18Z

t/process-master-pid.t

+--- request
+GET /t
+--- response_body_like chop
+\d+$


This test is too loose. We could do better here by comparing the number returned by master_pid() against the real master pid saved in the nginx pid file.

agentzh · 2018-01-02T00:10:48Z

t/process-master-pid.t

+            local process = require "ngx.process"
+
+            local v
+            local pid = process.master_pid


This is definitely wrong. You missed a pair of parentheses after process.master_pid. Have you actually run this test yourself?

This is another reason why you should make your PR pass the travis ci checks as @membphis suggested ;)

Sorry, I'm not familar with travis ci, I will try to correct it ,thanks.

And here is not an error ,because pid will be called in then next line ：
local v = pid()

@chronolaw I see, but the pid function name is indeed confusing. How about using the variable name get_pid here? So that there could be no ambiguity here.

chronolaw · 2018-01-02T03:27:18Z

I have run test in my own nginx 1.13.8+ngx_lua，and I will try to config travis ci after soon.

chronolaw · 2018-01-02T04:50:19Z

I have tried travis-ci build, but I got an error.

Then reason is that the process.master_pid needs nginx 1.13.8, but now OpenResty build will fail on it(no proper patch for it).

And in nginx 1.13.6, this API will return nil for no ngx_parent in nginx core.

Would you give me some advise to write the test case?

chronolaw · 2018-01-02T05:14:00Z

I created a new git branch to fit nginx 1.13.6, now process.master_pid passed ,but got another error

#   Failed test 'TEST 1: ngx.process.master_pid - pattern "(?^:\[TRACE   \d+ content_by_lua\(nginx\.conf:\d+\):4 loop\])" should match a line in error.log (req 0)'
#   at /home/travis/build/chronolaw/lua-resty-core/test-nginx/lib/Test/Nginx/Socket.pm line 1206.
#   Failed test 'TEST 1: ngx.process.master_pid - pattern "(?^:\[TRACE   \d+ content_by_lua\(nginx\.conf:\d+\):4 loop\])" should match a line in error.log (req 1)'

So could you tell my how to write the right error_log eval in xxx.t?

chronolaw · 2018-01-02T05:56:08Z

After some modify, my own test has passed, please see:https://travis-ci.org/chronolaw/lua-resty-core/jobs/324018825

Changes:
use chronolaw/lua-nginx-module.git and branch test_ffi_master_pid. In this branch code is

#if (nginx_version >= 1013008)
    return (int) ngx_parent;
#else
    return (int) ((ngx_process == NGX_PROCESS_SINGLE) ? ngx_pid : getppid());
    //return -1;
#endif

agentzh · 2018-01-02T19:58:21Z

@chronolaw You can skip the tests for nginx cores older than 1.13.8 with the --- skip_nginx section. You can see such examples in this library's existing test suite.

agentzh · 2018-01-02T20:00:38Z

@chronolaw And you should point to your own fork of lua-nginx-module in .travis.yml in your branch, see others' PRs for such examples, like this:

#89

agentzh · 2018-01-02T20:01:54Z

t/process-master-pid.t

+ stitch
+
+
+


We should not put extra blank lines at the end of the .t test files. The tool reindex can automatically format your .t files:

https://github.com/openresty/openresty-devel-utils/blob/master/reindex

agentzh · 2018-01-02T20:02:58Z

t/process-master-pid.t

+                ngx.say(false)
+            else
+                local str = f:read("*l")
+                ngx.say(v == tonumber(str or "0"))


For the sake of debugging test failures, I think we should also output the actual master pid in the .pid file in case of comparison failures.

agentzh · 2018-01-02T20:04:25Z

t/process-master-pid.t

+            local process = require "ngx.process"
+
+            local v
+            local pid = process.master_pid


@chronolaw I see, but the pid function name is indeed confusing. How about using the variable name get_pid here? So that there could be no ambiguity here.

agentzh · 2018-01-02T20:09:49Z

t/process-master-pid.t

+#worker_connections(1014);
+#master_process_enabled(1);
+#log_level('warn');
+


You need to add the following line here to enable nginx master process:

master_on();

The test scaffold does not turn on nginx master process by default. Without this line, this test file fails on my side:

t/process-master-pid.t .. 1/12 # Failed test 'TEST 1: ngx.process.master_pid - response_body_like - response is expected (false 69514)' # at /home/agentzh/git/lua-resty-core/../test-nginx/lib/Test/Nginx/Socket.pm line 1601. # 'false # 69514 # ' # doesn't match '(?^s:^true # \d+$)' # Failed test 'TEST 1: ngx.process.master_pid - response_body_like - response is expected (false 69514)' # at /home/agentzh/git/lua-resty-core/../test-nginx/lib/Test/Nginx/Socket.pm line 1601. # 'false # 69514 # ' # doesn't match '(?^s:^true # \d+$)' # Looks like you failed 2 tests of 12. t/process-master-pid.t .. Dubious, test returned 2 (wstat 512, 0x200) Failed 2/12 subtests Test Summary Report ------------------- t/process-master-pid.t (Wstat: 512 Tests: 12 Failed: 2) Failed tests: 2, 8 Non-zero exit status: 2 Files=1, Tests=12, 0 wallclock secs ( 0.01 usr 0.00 sys + 0.09 cusr 0.01 csys = 0.11 CPU) Result: FAIL

agentzh · 2018-01-02T20:12:34Z

t/process-master-pid.t

+                v = pid()
+            end
+
+            local f = io.open(ngx.config.prefix().."/logs/nginx.pid", "r")


Style: needs spaces around binary operators like ...

Also, io.open() may return an error, better wrap it with an assert() call.

agentzh · 2018-01-02T22:42:31Z

Merged by fixing the aforementioned issues myself. Thanks!

chronolaw and others added 4 commits November 7, 2017 14:29

small typo fix

2ba0da9

Merge pull request #1 from openresty/master

168e6a0

merge from latest openresty

feature: new API ngx.process.master_pid

bb7e723

tests: ngx.process.master_pid, needs review

b831619

agentzh requested changes Jan 2, 2018

View reviewed changes

chronolaw added 2 commits January 2, 2018 10:51

process.master_pid return nil if nginx_version < 1013008

390be52

tests: check nginx.pid for process.master_pid

91dc998

test case fixed in process-master-pid.t

a74fdb5

agentzh reviewed Jan 2, 2018

View reviewed changes

agentzh closed this Jan 2, 2018

chronolaw deleted the master_pid branch January 4, 2018 03:05

Conversation

chronolaw commented Dec 28, 2017

Uh oh!

membphis commented Dec 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chronolaw commented Jan 2, 2018

Uh oh!

chronolaw commented Jan 2, 2018

Uh oh!

chronolaw commented Jan 2, 2018

Uh oh!

chronolaw commented Jan 2, 2018

Uh oh!

agentzh commented Jan 2, 2018

Uh oh!

agentzh commented Jan 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agentzh commented Jan 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants