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

change: avoided running any init_by_lua* code inside signaller process and when testing Nginx configuration. #1377

Merged

Conversation

spacewander
Copy link
Member

Previously, when Nginx is run as a signaller like nginx -p . -s stop, the code in init_by_lua* will be executed. Now this probably unexpected behavior is avoided.

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

@thibaultcha
Copy link
Member

thibaultcha commented Sep 11, 2018

It should be doable to write a test for this behaviour with Test::Nginx (e.g. some side-effect triggered by init_by_lua*)? At least for the sake of preventing a future regression.

Food for thought:

Executing init_by_lua* in signaller processes has caused us some pain, so I am all in for this change. However, I can't help but think of:

  1. some application out there that depends on this behaviour for some reason,
  2. this behaviour actually being desired in the future to run Lua in signaller processes

We could address 1. with a new directive to optionally run the handler in signaller processes (lua_run_init_in_signaller on;). And 2. could be addressed with a new handler altogether, if we don't have the init_in_signaller on/off directive (e.g. signal_by_lua).

@spacewander spacewander changed the title change: avoided running any init_by_lua* code inside signaller process. change: avoided running any init_by_lua* code inside signaller process and when testing Nginx configuration. Sep 12, 2018
@spacewander
Copy link
Member Author

@thibaultcha
IMHO, people could run their Lua code via resty before calling nginx -s, so there is always an alternative.

Now tests are added, and I also disable running init_by_lua* when testing Nginx configuration in the new commits.

@thibaultcha
Copy link
Member

@spacewander Good call on -t and tests look fine to me 👍

}
}
--- no_error_log
[error]
Copy link
Member

Choose a reason for hiding this comment

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

@spacewander Mind renaming this test suite to 159-disable-init-by-lua.t? See #1394 which already deals with 2 test suites named 157-*.t.

@spacewander
Copy link
Member Author

@thibaultcha
Test file renamed.

@spacewander spacewander force-pushed the disable_init_by_lua_in_signal branch from 67cec36 to 1e6091e Compare October 23, 2018 03:47
…when testing the nginx configuration.

Signed-off-by: Thibault Charbonnier <[email protected]>
@thibaultcha thibaultcha force-pushed the disable_init_by_lua_in_signal branch from 1e6091e to a70f549 Compare January 7, 2019 19:49
@thibaultcha
Copy link
Member

I have pushed a new commit which fixes the false positive found in the first test case. It is less fancy, but more robust and simpler.

@Lekensteyn
Copy link

For future reference:
This issue can only be reproduced with (1) nginx -t, or (2) nginx -s ... when a block is not using lua_shared_dict:

if (!lmcf->requires_shm && lmcf->init_handler) {

When lua_shared_dict is enabled, the init handler will not be called at that point and will be processed at a later stage. This oversight was also the cause for a crash with nginx -t (#1462) which was fixed by #1463.

@thibaultcha
Copy link
Member

@Lekensteyn yes, I also have it on my TODO list already to fix a few other edge-cases that I believe aren' addressed yet. PRs welcome!

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.

4 participants