Skip to content

Conversation

@thibaultcha
Copy link
Member

@thibaultcha thibaultcha commented Apr 10, 2018

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

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

If all PRs are merged:

@thibaultcha
Copy link
Member Author

This PR is clearly missing tests and documentation, which I can add if desired.

void ngx_http_lua_ffi_process_signal_graceful_exit(void);
int ngx_http_lua_ffi_master_pid(void);
int ngx_http_lua_ffi_enable_sa_restart(int signum, int enabled);
char *strerror(int errnum);
Copy link
Member Author

Choose a reason for hiding this comment

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

imho, this might belong to another cdef definition, if so please let me know where.

Copy link
Member

Choose a reason for hiding this comment

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

This seems to break encapsulation. Better make the ngx_http_lua_ffi_enable_sa_restart() API function itself return the error string, just like our other C API functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem, will change it 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated. Is it looking good enough for tests and documentation?

@thibaultcha
Copy link
Member Author

@dndx @agentzh Hey there, any feedback on this side of the signal restart? I believe the ngx_lua part is reviewed, but the Lua-land and resty-cli aren't. This is a nice non-intrusive way to solve those resty-cli issues imho. Let me know if you get some time to look at it :)

void ngx_http_lua_ffi_process_signal_graceful_exit(void);
int ngx_http_lua_ffi_master_pid(void);
int ngx_http_lua_ffi_enable_sa_restart(int signum, int enabled);
char *strerror(int errnum);
Copy link
Member

Choose a reason for hiding this comment

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

This seems to break encapsulation. Better make the ngx_http_lua_ffi_enable_sa_restart() API function itself return the error string, just like our other C API functions?

@dndx
Copy link
Member

dndx commented May 3, 2018

@thibaultcha This PR LGTM. Although I am thinking if we could add some constants for common POSIX signals to make using this easier? Providing numbers directly is not always convenient.

See: http://man7.org/linux/man-pages/man7/signal.7.html section "Standard signals".

@thibaultcha
Copy link
Member Author

@dndx I thought about doing so but I wasn't sure where to put them (somewhere on the ngx global probably, but where?), and as such I wasn't sure it would be accepted. Do you have any suggestion?

@dndx
Copy link
Member

dndx commented May 4, 2018

@thibaultcha What about ngx.process.SIGPIPE? I think putting it as module level constant for process.lua is fine for now. @agentzh Any inputs on this as well?

@thibaultcha
Copy link
Member Author

@dndx If we hard-code them in the Lua-land, my only concern is portability. If we do so, should we only provide constants for those signals that are defined as the same across all architectures?

@dndx
Copy link
Member

dndx commented May 4, 2018

@thibaultcha Yes. I think we can just provide the ones in https://en.wikipedia.org/wiki/Signal_(IPC) that has "Portable number" assigned from POSIX and leave the rest to the user.

@thibaultcha
Copy link
Member Author

@dndx Works for me.

@thibaultcha
Copy link
Member Author

@dndx Added

@thibaultcha
Copy link
Member Author

@agentzh @dndx Ping on this and its sister PRs

@thibaultcha
Copy link
Member Author

Closing this PR as it seems the SA_RESTART flag will get an nginx directive, but not an FFI API (as per the review in openresty/lua-nginx-module#1296).

@thibaultcha thibaultcha closed this Nov 7, 2018
@thibaultcha thibaultcha deleted the feat/sa-restart branch November 7, 2018 01:59
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.

3 participants