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

Updating Fiber #517

Merged
merged 3 commits into from
Dec 18, 2023
Merged

Updating Fiber #517

merged 3 commits into from
Dec 18, 2023

Conversation

dearblue
Copy link
Contributor

間が空いてしまいすいません。
この PR によって mruby/mruby#6063 を閉じられるのではないかと思いますが、いかがでしょうか?

それと気付いたことがあったので、ひとつだけ補足をさせてください。
Nginx.redirect によって mrb_fiber_resume() が入れ子状に呼び出されることがあります。僕はこれまで問題がありそうだと考えていました。
しかしながら再考したところ Fiber#transfer が絡まなければ、コールスタックを圧迫すること以外は問題なさそうだと思うようになりました。
このことは mruby の mrb_fiber_resume() に関するドキュメント を更新する必要があります。
ただし、ちゃんとした検証を行ってからとなるので、mruby への提案は少し先になりそうです。

@matsumotory Please review.

Pull-Request Check List

@dearblue
Copy link
Contributor Author

🙀 もう一つ補足がありました。
現状ではテストが失敗する可能性があります。

build_config.rbconf.enable_debugconf.defines << "MRB_GC_STRESS" を追加することで再現できるはずです。

% sh test.sh

  ...略...

............................................XX....................................................
ArgumentError: ngx_mruby - rack base [location /rack_base_env] => string contains null byte
ArgumentError: ngx_mruby - rack base [method POST, location /rack_base_env] => string contains null byte
Total: 98
   OK: 96
   KO: 0
Crash: 2
 Time: 19.5605 seconds

これについては、mruby-json にパッチを当てることで成功するようになります。

mattn/mruby-json#49

@matsumotory
Copy link
Owner

ありがとうございます!mruby-jsonのPRがマージされ次第こちらでも確認します。

@dearblue dearblue marked this pull request as draft December 3, 2023 02:11
@dearblue
Copy link
Contributor Author

dearblue commented Dec 3, 2023

mruby 本体に C から扱う Fiber#resume / mrb_fiber_resume() 関連の問題がいくつか見つかりました。
この PR の範囲では不十分です。
mruby 側で (たぶん僕が) 修正するまでこの PR をドラフト状態にします。

@matsumotory
Copy link
Owner

了解です。とっても素晴らしい仕事ですね!ありがとうございます!!

Use direct functions to deal with fiber.
Workarounds are no longer required, by mruby/mruby@9ee7edc .
@dearblue
Copy link
Contributor Author

mruby 側の問題が修正されました。
C から Fiber#resume ができることを テストするようにした ので、障害はなくなったと思います (たぶん)。
もっとも、時期的に mruby 3.3 がリリースされても良い頃なので、正式版を待っても良さそうです。
ただし現時点でまだリリース候補が出ていないので、早くても年が明けてからのような気がします・・・・・・。

@dearblue dearblue marked this pull request as ready for review December 15, 2023 13:58
@matsumotory matsumotory merged commit 0d1bb85 into matsumotory:master Dec 18, 2023
@matsumotory
Copy link
Owner

Thanks!

@matsumotory
Copy link
Owner

ちかいうちにバージョンアップしておきますね

@pyama86
Copy link
Collaborator

pyama86 commented Dec 18, 2023

ありがとうございます!!!

@dearblue dearblue deleted the fiber branch December 18, 2023 13:35
@dearblue dearblue mentioned this pull request Mar 11, 2024
3 tasks
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