feature: introduced the 'lua_sa_restart' directive, which sets the SA_RESTART flag on nginx workers signal dispositions. #1296

thibaultcha · 2018-04-10T21:25:03Z

This is one of 4 PRs to properly handle EINTR errors in resty-cli (and/or OpenResty if desired by the user).

Revert I/O retry on EINTR patch luajit2#18 Revert the "retry on EINTR" patch
(this PR) Implement an FFI API to enable SA_RESTART on signal handlers
feature: added the sig_restart API to the ngx.process module. lua-resty-core#183 Implement the Lua sig_restart() API
tests: added more test cases for retries on interrupted file I/O. resty-cli#41 Enable the SA_RESTART flag on interrupting signals

If all PRs are merged:

we get rid of the "retry on EINTR" LuaJIT patch which was refused upstream
resty-cli users can safely use APIs like io.popen() popen crashes in resty but not in luajit resty-cli#35
OpenResty users can toggle this behaviour on/off as desired (off by default)

src/ngx_http_lua_worker.c

agentzh · 2018-06-29T19:07:00Z

src/ngx_http_lua_worker.c

+    struct sigaction        act;
+
+    if (ngx_process != NGX_PROCESS_WORKER
+        && ngx_process != NGX_PROCESS_SINGLE)


We need to take into account the privileged agent helper process here.

agentzh · 2018-06-29T19:08:50Z

@thibaultcha I think it's better to simply enable SA_RESTART in our lua module's init worker hook on the C land? It's a burden for the user to always remember calling a Lua API in their own init_worker_by_lua* handler.

thibaultcha · 2018-06-29T20:56:10Z

@agentzh Does this list sound good for the set of signals the SA_RESTART flag is to be enabled by default: https://github.com/openresty/resty-cli/pull/41/files#diff-f40cddd047a559bb89d08b41dfdfd86dR625 ?

thibaultcha · 2018-06-30T02:15:40Z

I must admit that for the sake of backwards-compatibility (in the event of an unforeseen issue), I was under the impression that we would not enable this flag on any signal by default - especially considering it is enabled by default in resty-cli (as per openresty/resty-cli#41), where it matters the most.

But I am fine enabling it by default in this module's init context, no problem (if the suggested list of signals is good to go).

agentzh · 2018-06-30T17:08:08Z

@thibaultcha That list looks good to me. This is also important for non-resty-cli contexts. I don't like enabling by hand, which is too easy to miss for average users. And it does not look like there is any unwanted side effects so there's no backward compatibility that we need to preserve here?

dndx · 2018-06-30T18:23:29Z

@thibaultcha I think at least we should include the signals found in http://lxr.nginx.org/source/src/os/unix/ngx_process.c#0039. The last two are not needed since they are ignored already.

thibaultcha · 2018-07-04T04:23:45Z

@agentzh @dndx I have reworked the PR and other related ones. So far, this PR sets the SA_RESTART flag upon worker initialization, and exposes an FFI API that openresty/lua-resty-core#183 can consume. openresty/resty-cli#41 now simply adds regression tests for the EINTR error on file handle reads (still needs docs & tests if we are exposing the Lua-land API).

I do wonder if maintaining a patch adding the SA_RESTART flag in ngx_init_signals would be easier? But the FFI API is a nice-to-have anyway I think.

thibaultcha · 2018-08-01T21:19:01Z

@agentzh @dndx Ping here :)

Also maybe #1239 as well, which I still consider quite important for true Lua compatibility.

thibaultcha · 2018-09-05T02:26:32Z

Patch updated:

introduced a lua_sa_restart directive (on by default)
removed ngx_http_lua_set_sa_restart_helper(int signo, int enabled, u_char *err, size_t *errlen); now, the SA_RESTART flag is directly set by ngx_http_lua_set_sa_restart (which cannot remove the flag)
removed ngx_http_lua_ffi_set_sa_restart (which invalidates feature: added the sig_restart API to the ngx.process module. lua-resty-core#183)

I did like the granularity offered by the FFI API (being able to set/unset this flag as desired on a per-signal basis), but I removed it to show what a minimal patch would look like. I can add the FFI back any time if desired.

src/ngx_http_lua_module.c

agentzh · 2018-10-24T17:45:03Z

src/ngx_http_lua_util.c

@@ -70,6 +70,25 @@
 #endif


+#if !(NGX_WIN32)


I suggest we do feature test instead of excluding just win32 here for better portability.

thibaultcha · 2018-11-07T02:15:05Z

@agentzh Updated with a feature test. Mind having another look? If ready, I will port this PR to the meta-lua module as well.

agentzh

Should add some docs. Otherwise it looks good to me.

thibaultcha · 2018-11-08T04:43:30Z

Pushed some documentation.

agentzh · 2018-11-12T19:17:45Z

src/ngx_http_lua_util.c

+                       "setting SA_RESTART for signal %d", *signo);
+
+        if (sigaction(*signo, NULL, &act) != 0) {
+            ngx_log_error(NGX_LOG_WARN, log, 0, "failed to get sigaction for "


Should use the errno arg directly.

@agentzh Updated

… SA_RESTART flag on nginx workers signal dispositions.

This was referenced Apr 10, 2018

Revert I/O retry on EINTR patch openresty/luajit2#18

Merged

feature: added the sig_restart API to the ngx.process module. openresty/lua-resty-core#183

Closed

tests: added more test cases for retries on interrupted file I/O. openresty/resty-cli#41

Closed

agentzh requested changes Apr 15, 2018

View reviewed changes

src/ngx_http_lua_worker.c Outdated Show resolved Hide resolved

src/ngx_http_lua_worker.c Outdated Show resolved Hide resolved

thibaultcha force-pushed the feat/sa-restart branch from 358763e to 9f2d3b3 Compare April 16, 2018 18:06

dndx approved these changes Apr 17, 2018

View reviewed changes

thibaultcha force-pushed the feat/sa-restart branch from 9f2d3b3 to aba20d0 Compare May 3, 2018 22:21

thibaultcha mentioned this pull request Jun 28, 2018

"io.popen:read()" return nil sometimes in "openresty + osx" #1346

Closed

agentzh requested changes Jun 29, 2018

View reviewed changes

thibaultcha force-pushed the feat/sa-restart branch from aba20d0 to fd3b236 Compare July 4, 2018 04:02

thibaultcha force-pushed the feat/sa-restart branch 2 times, most recently from 9714bb0 to 2598710 Compare July 4, 2018 05:44

thibaultcha mentioned this pull request Jul 17, 2018

error when using with OpenResty cli lunarmodules/luacov#60

Closed

thibaultcha force-pushed the feat/sa-restart branch 3 times, most recently from 5a7e66b to f928849 Compare September 5, 2018 02:20

agentzh requested changes Oct 24, 2018

View reviewed changes

thibaultcha force-pushed the feat/sa-restart branch from f928849 to 92f4e62 Compare November 7, 2018 02:13

thibaultcha changed the title ~~feature: added an FFI API for setting the SA_RESTART flag on signals~~ feature: introduced the 'lua_sa_restart' directive, which sets the SA_RESTART flag on nginx workers signal dispositions. Nov 7, 2018

agentzh approved these changes Nov 7, 2018

View reviewed changes

thibaultcha force-pushed the feat/sa-restart branch from 92f4e62 to e6be59f Compare November 8, 2018 04:42

thibaultcha force-pushed the feat/sa-restart branch 3 times, most recently from d6d6673 to 67e118b Compare November 8, 2018 05:48

thibaultcha mentioned this pull request Nov 8, 2018

feature: implemented a new 'lua_sa_restart' directive, which sets the SA_RESTART flag on nginx workers signal dispositions. openresty/meta-lua-nginx-module#53

Merged

agentzh requested changes Nov 12, 2018

View reviewed changes

feature: implemented a new 'lua_sa_restart' directive, which sets the…

fbf47fa

… SA_RESTART flag on nginx workers signal dispositions.

thibaultcha force-pushed the feat/sa-restart branch from 67e118b to fbf47fa Compare November 12, 2018 21:36

agentzh approved these changes Nov 12, 2018

View reviewed changes

thibaultcha merged commit 2f7c650 into openresty:master Nov 13, 2018

thibaultcha deleted the feat/sa-restart branch November 13, 2018 02:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: introduced the 'lua_sa_restart' directive, which sets the SA_RESTART flag on nginx workers signal dispositions. #1296

feature: introduced the 'lua_sa_restart' directive, which sets the SA_RESTART flag on nginx workers signal dispositions. #1296

thibaultcha commented Apr 10, 2018 •

edited

Loading

agentzh Jun 29, 2018

agentzh commented Jun 29, 2018 •

edited

Loading

thibaultcha commented Jun 29, 2018

thibaultcha commented Jun 30, 2018

agentzh commented Jun 30, 2018

dndx commented Jun 30, 2018

thibaultcha commented Jul 4, 2018

thibaultcha commented Aug 1, 2018

thibaultcha commented Sep 5, 2018

agentzh Oct 24, 2018

thibaultcha commented Nov 7, 2018

agentzh left a comment

thibaultcha commented Nov 8, 2018

agentzh Nov 12, 2018

thibaultcha Nov 12, 2018

feature: introduced the 'lua_sa_restart' directive, which sets the SA_RESTART flag on nginx workers signal dispositions. #1296

feature: introduced the 'lua_sa_restart' directive, which sets the SA_RESTART flag on nginx workers signal dispositions. #1296

Conversation

thibaultcha commented Apr 10, 2018 • edited Loading

agentzh Jun 29, 2018

Choose a reason for hiding this comment

agentzh commented Jun 29, 2018 • edited Loading

thibaultcha commented Jun 29, 2018

thibaultcha commented Jun 30, 2018

agentzh commented Jun 30, 2018

dndx commented Jun 30, 2018

thibaultcha commented Jul 4, 2018

thibaultcha commented Aug 1, 2018

thibaultcha commented Sep 5, 2018

agentzh Oct 24, 2018

Choose a reason for hiding this comment

thibaultcha commented Nov 7, 2018

agentzh left a comment

Choose a reason for hiding this comment

thibaultcha commented Nov 8, 2018

agentzh Nov 12, 2018

Choose a reason for hiding this comment

thibaultcha Nov 12, 2018

Choose a reason for hiding this comment

thibaultcha commented Apr 10, 2018 •

edited

Loading

agentzh commented Jun 29, 2018 •

edited

Loading