Implement mruby_ssl_handshake_handler() #205

Merged
merged 7 commits into from Aug 24, 2016

Projects

None yet

2 participants

@hfm
Collaborator
hfm commented Aug 22, 2016

This function is equivalent to mruby_ssl_handshake_handler_code() #145, except that the file specified by ngx_mruby-script contains the mruby code to be executed.

@matsumotory
Owner

@hfm Thank you for you PR. The base code looks good to me. So, please consider the following implementations:

  • create new ngx_mrb_code_t *ssl_handshake_inline_code for mruby_ssl_handshake_handler
  • for now, ngx_mrb_code_t *ssl_handshake_inline_code share ngx_str_t *servername. ngx_str_t cert_path and ngx_str_t cert_key_path with ngx_mrb_code_t *ssl_handshake_code

Your code share the byte-code between mruby_ssl_handshake_handler_code and mruby_ssl_handshake_handle directive.

I want to merge the PR after implementing my request.

@hfm
Collaborator
hfm commented Aug 23, 2016

@matsumoto-r Thank you for yours advice. I tried to revise it by 29dff95. Would you please review it?

@matsumotory matsumotory and 1 other commented on an outdated diff Aug 23, 2016
src/http/ngx_http_mruby_module.c
@@ -2474,7 +2524,11 @@ static int ngx_http_mruby_ssl_cert_handler(ngx_ssl_conn_t *ssl_conn, void *data)
mrb = mscf->state->mrb;
mrb->ud = mscf;
ai = mrb_gc_arena_save(mrb);
- mrb_run(mrb, mscf->ssl_handshake_code->proc, mrb_top_self(mrb));
+ if (mscf->ssl_handshake_code != NGX_CONF_UNSET_PTR) {
+ mrb_run(mrb, mscf->ssl_handshake_code->proc, mrb_top_self(mrb));
+ } else {
+ mrb_run(mrb, mscf->ssl_handshake_inline_code->proc, mrb_top_self(mrb));
+ }
@matsumotory
matsumotory Aug 23, 2016 Owner

When both ssl_handshake_code and ssl_handshake_inline_code have byte-code, these both byte-codes should run at this phase.

@hfm
hfm Aug 23, 2016 Collaborator

Indeed. I'll fix it.

@matsumotory
matsumotory Aug 23, 2016 Owner

When don't use code cache (arg 3 = "cache"), we should recompile the ruby code, like the following:

ref: src/http/ngx_http_mruby_module.c

1879     if (!code->cache) {                                                                                                \
1880       NGX_MRUBY_STATE_REINIT_IF_NOT_CACHED(mlcf->cached, mmcf->state, code, ngx_http_mruby_state_reinit_from_file);    \
1881     }                                                                                                                  \
                                                                                                            \                                                                                                      
@matsumotory
Owner

@hfm These all changes looks good to me!! If you don't have other confirmations, I'll merged this.

@hfm hfm Add test whether byte-code is cached in ssl certificate changing
If not caching code, when you change the code, ngx_mruby should recompile it without reloading nginx.
018ab35
@hfm
Collaborator
hfm commented Aug 24, 2016

I added additional tests by 018ab35.

Besides, I checked memory increasing tendency, and I might have found leaking memory.

# Startup
$ ps aux | grep '[n]ginx'
hfm      22988   0.0  0.1  2468092   5248 s001  S+    2:05PM   0:00.05 build/nginx/sbin/nginx
# Start `h2load -c 300 -n 500000 https://localhost:58085/`
$ ps aux | grep '[n]ginx'
hfm      22988  85.9  0.2  2470140  15768 s001  R+    2:05PM   0:01.29 build/nginx/sbin/nginx
$ ps aux | grep '[n]ginx'
hfm      22988  91.6  0.2  2470140  16860 s001  R+    2:05PM   0:04.31 build/nginx/sbin/nginx
$ ps aux | grep '[n]ginx'
hfm      22988  89.4  0.2  2470140  17584 s001  R+    2:05PM   0:07.30 build/nginx/sbin/nginx
$ ps aux | grep '[n]ginx'
hfm      22988  91.5  0.2  2470140  17692 s001  R+    2:05PM   0:10.39 build/nginx/sbin/nginx
# Finish `h2load -c 300 -n 500000 https://localhost:58085/`
$ ps aux | grep '[n]ginx'
hfm      22988   3.7  0.2  2470140  17792 s001  S+    2:05PM   0:13.89 build/nginx/sbin/nginx

But this occurs also in master branch. I think we should try to solve it in other PR.

@matsumotory
Owner

@hfm Thank you for your investigation! I agree with your thought. I merged this. We'll investigate the memory issue as other PR.

@matsumotory matsumotory merged commit 94cf22f into matsumotory:master Aug 24, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hfm hfm deleted the hfm:mruby_ssl_handshake_handler branch Aug 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment