Skip to content

New Feature: added args support for ngx.on_abort #269

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guanlan
Copy link

@guanlan guanlan commented Aug 13, 2013

No description provided.

@bakins
Copy link
Member

bakins commented Aug 13, 2013

In typical Lua usage, one would just pass a closure to on_abort rather than using pass values:

 ngx.on_abort(function() someotherfunction(my, arguments, for, the, other, function) end )

@guanlan
Copy link
Author

guanlan commented Aug 13, 2013

Because we need keep consistent with ngx.timer.at API.

@agentzh
Copy link
Member

agentzh commented Aug 13, 2013

@bakins Creating new closures at every request does introduce some overhead (observable in on-CPU time Flame Graphs, for example), so I think we need the ability to pass user arguments just as in ngx.timer.at().

@bakins
Copy link
Member

bakins commented Aug 16, 2013

@agentzh I'm not surprised by the creation of a closure on each request showing up in a flame graph, I just wonder how much difference it makes in the real world. I don't have any strong feelings either way other than using a closure is more "Lua like."

@agentzh
Copy link
Member

agentzh commented Aug 19, 2013

@bakins Creating closures per request could be a real bottleneck, as observed as 20% ~ 30% overall overhead in an on-CPU time flame graph for our Lua WAF system. Even though in that case it was not ngx.on_abort() in particular, but more aggressive use of "higher-order functions".

@mergify
Copy link

mergify bot commented Jun 26, 2020

This pull request is now in conflict :(

@mergify
Copy link

mergify bot commented Mar 20, 2023

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Mar 20, 2023
@mergify mergify bot removed the conflict label May 10, 2023
@mergify
Copy link

mergify bot commented May 10, 2023

This pull request is now in conflict :(

@mergify mergify bot added the conflict label May 10, 2023
@mergify mergify bot removed the conflict label Sep 23, 2023
@mergify
Copy link

mergify bot commented Sep 23, 2023

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Sep 23, 2023
@mergify mergify bot removed the conflict label Mar 6, 2024
Copy link

mergify bot commented Mar 6, 2024

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Mar 6, 2024
@mergify mergify bot removed the conflict label May 27, 2024
Copy link

mergify bot commented May 27, 2024

This pull request is now in conflict :(

@mergify mergify bot added the conflict label May 27, 2024
@mergify mergify bot removed the conflict label Feb 13, 2025
Copy link

mergify bot commented Feb 13, 2025

This pull request is now in conflict :(

@mergify mergify bot added the conflict label Feb 13, 2025
@mergify mergify bot removed the conflict label May 7, 2025
Copy link

mergify bot commented May 7, 2025

This pull request is now in conflict :(

@mergify mergify bot added the conflict label May 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants