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

Issue 151 #191

Merged
merged 1 commit into from
Aug 3, 2016
Merged

Issue 151 #191

merged 1 commit into from
Aug 3, 2016

Conversation

yyamano
Copy link
Collaborator

@yyamano yyamano commented Jul 27, 2016

The changes fixes #151

@matsumotory
Copy link
Owner

ありがとうございます。context_freeはmrb_runが終わった後に実施しないとfileの行などのcontextが適切に取れないと思うので、mrb_run後やエラー処理時に必ずngx_mrb_code_cleanを呼んだり、request_recを保持していない場合はmrb_context_freeを単体で呼ぶべきだと思うのですがいかがでしょうか。

@yyamano
Copy link
Collaborator Author

yyamano commented Jul 27, 2016

mrbc_context なのでコンパイル時にしか必要ないと思っていたんですが、どこで使ってるんでしょうか?

@matsumotory
Copy link
Owner

通常は必要ありませんが、ngx_mrubyのようなstateを共有しているプログラムやnginx.confの行数とRubyコードの行数をコントロールしたい場合においては、cxtを保持してlinenoeなどを変更して再利用してparseしたいといった要求があるので、よっぽど積極的な理由がないかぎりはコードの保守性を考えて、バイトコード実行後にfreeする実装で良いと思います。

@yyamano
Copy link
Collaborator Author

yyamano commented Jul 27, 2016

ngx_int_t ngx_http_mruby_state_reinit_from_file() のことでしょうか?
そうだとすると、ngx_mrb_gencode_state()の中で mrbc_context_new() しているので、mrbc_context の再利用はされないんじゃないでしょうか。

僕は ngx_mruby の内部構造を完全に把握しているわけではないので、松本さんの指摘の意味を理解できていないです:-( お手数かけてすみません。

@matsumotory
Copy link
Owner

あ、いえ、沢山ご協力頂いてこちらとしては感謝しかありませんので気になさらないでください!

mrbc_contextについて、現在再利用はあまりできていませんが、今後の方針として行数を書き換えたりしていきたいという方向性があって(例えばmrb_runした後にfiberで待たせて、もう一度mrb_runしたいとか、mrb_runを複数回叩く方向性を考えている)、よっぽど積極的な理由がない限りは今後のcontext再利用を踏まえて、mrb_run後の最後のcleanup処理で実行しておきたい、という意図です。

@yyamano
Copy link
Collaborator Author

yyamano commented Jul 28, 2016

なるほど、了解です。今日は時間が取れないので、明日以降、変更してみます。

* Free mbc_context after calling mrb_run, and at stop time.
* Add missing MRB_SET_INSTANCE_TT() with MRB_TT_DATA.
@yyamano
Copy link
Collaborator Author

yyamano commented Aug 3, 2016

遅くなりましたが修正してみました。

追記: コミットをまとめたらよくわかんなくなっちゃいましたね。この変更です。 yyamano@2fcaef7

@matsumotory
Copy link
Owner

大丈夫です、コードが綺麗なのでわかります〜。グレートワーク!thanks!

@matsumotory matsumotory merged commit 2c1f770 into matsumotory:master Aug 3, 2016
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.

Nginx::Upstream memory leak?
2 participants