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) ensure 'kong.ctx.plugin' and 'kong.log' are light-threads safe #5873

Merged
merged 4 commits into from
May 14, 2020

Conversation

thibaultcha
Copy link
Member

This PR replaces #5851 because it provides a more robust implementation and fixes several other issues.

kong.ctx

  • Store namespace keys in ngx.ctx to ensure all dynamically generated namespaces are isolated on a per-request basis.
  • Introduce a new global API (which is a private API for use by core only) to explicitly delete a namespace.

A note on the benefits of this new implementation compared to #5851:

  • By using a table to keep track of namespaces in ngx.ctx, we ensure that users of ngx.ctx cannot access the table namespaces, thus properly isolating it and avoiding polluting ngx.ctx for core and/or plugins. We can think of it as a double-pointer reference to the namespace. Each request gets a pre-allocated table for 4 namespaces (of course not limited to 4), with the assumption that most instances do not execute more than 4 plugins using kong.ctx.plugin at a time.
  • We ensure that namespace key references cannot be nil to ensure a safe usage from the core.
  • All namespace key references are still weak when assigned to a namespace, thus tying the lifetime of the namespace to that of its key reference. Similarly to the previous implementation, this is done to
    ensure we avoid potential memory leaks. That said, the kong.ctx.plugin namespace does use the plugin's conf for that purpose anymore (See 40dc146), which alleviates such concerns in the current usage of this API.
  • All tables allocated for namespace key references and namespaces keys themselves will be released when ngx.ctx will be GC'ed.
  • We also ensure than kong.ctx.* returns nil when ngx.ctx is not supported ("init" phase).

kong.log

  • Store current logging facility in kong.ctx.core to ensure that multiple concurrent request do not share the same kong.log Lua global facility.
  • Log facilities are still cached in a local upvalue to avoid re-recreating them for every incoming request.

A note on the current implementation: unlike the kong.ctx PDK module, the kong.log module does not encapsulate its namespacing capabilities. Doing so would be more efficient, as we would avoid having to create multiple kong.log instances. Instead, a singleton instance could keep track of the different namespaces (and their assigned logging formats) itself.

Fix #4379
Fix #5853
Fix #5057
See #5851
See #5459

Copy link
Member

@kikito kikito left a comment

Choose a reason for hiding this comment

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

Most of my comments were cosmetic / low importance. But I think the changes in the tests might be needed.

kong/pdk/ctx.lua Outdated Show resolved Hide resolved
kong/pdk/ctx.lua Outdated Show resolved Hide resolved
kong/pdk/ctx.lua Show resolved Hide resolved
kong/pdk/ctx.lua Show resolved Hide resolved
kong/pdk/ctx.lua Show resolved Hide resolved
t/02-global/02-set-named-ctx.t Show resolved Hide resolved
t/02-global/02-set-named-ctx.t Show resolved Hide resolved
t/02-global/03-namespaced_log.t Show resolved Hide resolved
* Store namespace keys in `ngx.ctx` to ensure all dynamically generated
  namespaces are isolated on a per-request basis.
* Introduce a new global API (which is a private API for use by core
  only) to explicitly delete a namespace.

A note on the benefits of this new implementation:

* By using a table to keep track of namespaces in `ngx.ctx`, we ensure
  that users of `ngx.ctx` cannot access the table namespaces, thus
  properly isolating it and avoiding polluting `ngx.ctx` for core and/or
  plugins. We can think of it as a double-pointer reference to the
  namespace. Each request gets a pre-allocated table for 4 namespaces
  (of course not limited to 4), with the assumption that most instances
  do not execute more than 4 plugins using `kong.ctx.plugin` at a time.
* We ensure that namespace key references cannot be `nil` to ensure a
  safe usage from the core.
* All namespace key references are still weak when assigned to a
  namespace, thus tying the lifetime of the namespace to that of its key
  reference. Similarly to the previous implementation, this is done to
  ensure we avoid potential memory leaks. That said, the
  `kong.ctx.plugin` namespace does use the plugin's conf for that
  purpose anymore (See 40dc146), which
  alleviates such concerns in the current usage of this API.
* All tables allocated for namespace key references and namespaces keys
  themselves will be released when `ngx.ctx` will be GC'ed.
* We also ensure than `kong.ctx.*` returns `nil` when `ngx.ctx` is not
  supported ("init" phase).

Co-Authored-By: tdelaune <[email protected]>

Fix #4379
Fix #5853
See #5851
See #5459
* Store current logging facility in `kong.ctx.core` to ensure that
  multiple concurrent request do not share the same `kong.log` Lua
  global facility.
* Log facilities are still cached in a local upvalue to avoid
  re-recreating them for every incoming request.

A note on the current implementation: unlike the `kong.ctx` PDK module,
the `kong.log` module does not encapsulate its namespacing capabilities.
Doing so would be more efficient, as we would avoid having to create
multiple `kong.log` instances. Instead, a singleton instance could keep
track of the different namespaces (and their assigned logging formats)
itself.

Fix #5057
Changes:

* fix light thread safety issues in `kong.ctx`
* fix light thread safety issues in `kong.log`
* minor styling and typo fixes
@thibaultcha thibaultcha force-pushed the fix/pdk-light-thread-safe branch from f02e975 to 30df838 Compare May 14, 2020 17:33
@thibaultcha
Copy link
Member Author

@kikito Thanks! I pushed a revised version, will wait until CI yields green again and merge.

@thibaultcha thibaultcha merged commit 75479d9 into master May 14, 2020
@thibaultcha thibaultcha deleted the fix/pdk-light-thread-safe branch May 14, 2020 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants