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

fix(pdk/vault): do not call sema:wait in init phase #9851

Merged
merged 2 commits into from
Dec 1, 2022

Conversation

chronolaw
Copy link
Contributor

@chronolaw chronolaw commented Dec 1, 2022

Summary

Fix issue #9839

Checklist

  • The Pull Request has tests
  • There's an entry in the CHANGELOG

Full changelog

  • check nginx phase
  • if it is init or init_worker, set wait_ok to false

Issue reference

Fix #[9839]

@chronolaw
Copy link
Contributor Author

I think it should have a change log entry, but I don't know how to describe it.
Could someone help me for it?

@mong0520
Copy link

mong0520 commented Dec 1, 2022

hi @chronolaw ,

I'm the issue submitter of #9839.
First thank you for the fixing, and I have few questions about this PR.

  1. Since sema:wait only works in some specific phases, how about set wait_ok to true if those phases are detected instead of set it to false in init false only?
# https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/semaphore.md#wait
syntax: ok, err = sema:wait(timeout)
context: rewrite_by_lua*, access_by_lua*, content_by_lua*, ngx.timer.*
  1. Won't it be any problem if not calling to RETRY_SEMAPHORE:wait(RETRY_WAIT) but calls to RETRY_SEMAPHORE:post() in subsequence codes? I don't know if they should be paired.

@chronolaw
Copy link
Contributor Author

hi @chronolaw ,

I'm the issue submitter of #9839. First thank you for the fixing, and I have few questions about this PR.

  1. Since sema:wait only works in some specific phases, how about set wait_ok to true if those phases are detected instead of set it to false in init false only?
# https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/semaphore.md#wait
syntax: ok, err = sema:wait(timeout)
context: rewrite_by_lua*, access_by_lua*, content_by_lua*, ngx.timer.*
  1. Won't it be any problem if not calling to RETRY_SEMAPHORE:wait(RETRY_WAIT) but calls to RETRY_SEMAPHORE:post() in subsequence codes? I don't know if they should be paired.
  1. In Kong, pdk code will run in init init_worker access rewrite phases, I think it is OK.
  2. We set wait_ok = false, so subsequence code will not call RETRY_SEMAPHORE:post()

@chronolaw chronolaw requested a review from bungle December 1, 2022 02:55
@chronolaw chronolaw force-pushed the fix/semaphore_cant_work_when_init branch from 6baa483 to 185598f Compare December 1, 2022 06:20
@bungle bungle merged commit 62cbf1d into master Dec 1, 2022
@bungle bungle deleted the fix/semaphore_cant_work_when_init branch December 1, 2022 09:38
@bungle
Copy link
Member

bungle commented Dec 1, 2022

@chronolaw thank you!

@mong0520
Copy link

mong0520 commented Dec 1, 2022

@chronolaw
One more question.
It looks like an always crash issue (calls to sema:wait() in init phase), but why there is no one reports it since 3.0.1 was released for a month?

oowl pushed a commit that referenced this pull request Aug 15, 2024
Bumps [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node) from 18.19.41 to 18.19.42.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node)

---
updated-dependencies:
- dependency-name: "@types/node"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Hayk <48865647+Hayk-S@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants