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.

Sign up for GitHub

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

Jump to bottom

fix(core) ctx dropped in log phase #8604

Merged
merged 6 commits into from
Apr 7, 2022
Merged

fix(core) ctx dropped in log phase #8604

merged 6 commits into from
Apr 7, 2022

Conversation

StarlightIbuki
Copy link
Contributor

Log phase does recover ngx.ctx for some of functions(with wrapper function), however for those functions not wrapped, they will see an empty ngx.ctx, and cause an unreadable error message for check_phase.
This is an ad hoc fix for check_phase.

Fix #8598

@StarlightIbuki StarlightIbuki mentioned this pull request Mar 29, 2022
Closed
@StarlightIbuki
Copy link
Contributor Author

For people who want to review this PR:
We handle log phase RPC in a timer because we are using cosocket for RPC that is not available in the log phase.
We did save the ngx.ctx object but if we recover it unconditionally it causes new problems(and ngx.ctx is not stable).

@ADD-SP
Copy link
Contributor

ADD-SP commented Mar 30, 2022

Should we keep the context for all phases? This way seems a bit unreliable. Is there any other way that we can share data for all phases, except shared memory and database?

@StarlightIbuki
Copy link
Contributor Author

@fffonion suggests recovering ngx.ctx in the timer instead of in mp_rpc.lua, as this should be transparent to the rpc implementation, and apply both for mp and pb.

fffonion
fffonion approved these changes Apr 6, 2022
View reviewed changes
Copy link
Contributor

@fffonion fffonion left a comment

Choose a reason for hiding this comment

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

minor comment, otherwise look good to me. feel free to merge after addressed (and squashed)

StarlightIbuki and others added 6 commits April 7, 2022 15:44
@StarlightIbuki
fix(core) ctx dropped in log phase
446d92d 
Log phase does recover ngx.ctx for some of functions(with wrapper function), however for those functon not wrapped, they will see an empty ngx.ctx, and cause unreadable error message for `check_phase`.
This is an ad hoc fix for `check_phase`.

Fix #8598
@StarlightIbuki
fix(core): check table existance first
dcbd273
@StarlightIbuki
Revert "fix(core): check table existance first"
4a87e1d 
This reverts commit ad4ac2b.
@StarlightIbuki
fix(core) fix should be done from caller
26fb183
@StarlightIbuki
Revert "fix(core) ctx dropped in log phase"
ad36b32 
This reverts commit 6d53fa0.
@StarlightIbuki @fffonion
apply style suggestion
8882efc 
Co-authored-by: Wangchong Zhou <[email protected]>
@StarlightIbuki
Copy link
Contributor Author

Rebased the PR. Hope that will fix the test.

@StarlightIbuki StarlightIbuki merged commit fe4bb60 into master Apr 7, 2022
@StarlightIbuki StarlightIbuki deleted the fix/empty_ctx_phase_adhoc branch April 7, 2022 08:09
@StarlightIbuki StarlightIbuki mentioned this pull request Jun 2, 2022
Closed
1 task
@fffonion fffonion mentioned this pull request Oct 10, 2022
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fffonion fffonion fffonion approved these changes

@javierguerragiraldez javierguerragiraldez Awaiting requested review from javierguerragiraldez

@ADD-SP ADD-SP Awaiting requested review from ADD-SP

Assignees
No one assigned
Labels
core/language/go core/language/js core/language/python size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[bug] plugin log phase handler causes error in PDK
3 participants
@StarlightIbuki @ADD-SP @fffonion