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(core): use require implementation written in Lua #11610

Closed
wants to merge 1 commit into from

Conversation

hanshuebner
Copy link
Contributor

@hanshuebner hanshuebner commented Sep 19, 2023

Summary

Some of the libraries that we use perform socket functions while they are being required(). This causes problems if the initial require() of the module is done from running code (instead of on the module global level) as require is implemented in C, and yielding is not possible across the boundaries of a FFI invocation.

This commit replaces the require() implementation by one that is written in Lua so that yielding is possible on the module global level.

Checklist

  • The Pull Request has tests
  • A changelog file has been added to CHANGELOG/unreleased/kong or adding skip-changelog label on PR if unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

KAG-2595

@chronolaw chronolaw changed the title fix(*): use require implementation written in Lua fix(core): use require implementation written in Lua Sep 20, 2023
@hanshuebner hanshuebner force-pushed the use-lua-require branch 2 times, most recently from 6ca5038 to 16a6292 Compare September 20, 2023 05:32
@hanshuebner hanshuebner marked this pull request as ready for review September 20, 2023 05:33
@github-actions github-actions bot added the author/community PRs from the open-source community (not Kong Inc) label Sep 20, 2023
Some of the libraries that we use perform socket functions while they
are being required().  This causes problems if the initial require() of
the module is done from running code (instead of on the module global
level) as require is implemented in C, and yielding is not possible
across the boundaries of a FFI invocation.

This commit replaces the require() implementation by one that is
written in Lua so that yielding is possible on the module global
level.
@chronolaw
Copy link
Contributor

I have a question: what is the performance of "lua require"? Is it slower or faster than "C require"?

@hanshuebner
Copy link
Contributor Author

I have a question: what is the performance of "lua require"? Is it slower or faster than "C require"?

I would expect it to be slower, but as the number of modules that we require is bounded and we don't require repeatedly in the hot path, it is better to have a robust solution that is a little slower than a fast one that can break in very obscure ways.

@bungle
Copy link
Member

bungle commented Sep 21, 2023

There is no immediate need for this at the moment. I feel we may want to discuss more about this. The ”resty.signal” LUA_CPATH parsing issue made it clear that it is quite easy to get things wrong or different compared to native. I think it is better to hold this, and not try to rush it in. Perhaps we don’t even want it. Yield across C boundary error is in genaral good indication to refactor the code so that it won’t happen, in my opinion.

@chronolaw chronolaw added the pr/discussion This PR is being debated. Probably just a few details. label Sep 21, 2023
@hanshuebner
Copy link
Contributor Author

Yield across C boundary error is in genaral good indication to refactor the code so that it won’t happen

I generally agree that top-level module code should not require. The issue with the "Yield across C boundary" error is that it can leave back the heap in a corrupted state from which there is no recovery, and we have seen situations where the error has not even been printed to a log file. Given the risk that customers see segmentation violations, even during plugin development time, I think that it is better to merge this PR and see if there are any issues during testing. If there are none, we can still write our code so that require does not perform yielding operations.

Another alternative would be to make the "Yield across C boundary" error exit the process instead of reporting it as a normal error. This would require a patch to LuaJIT, however, and that seems to be more risky a change.

@dndx
Copy link
Member

dndx commented Sep 22, 2023

I suggest we do not do on this change. There has been 0 reports of this bug previously and it seems that we are making a rather large change to address a very remote possibility. Yes it could be bad when it does happen, but at the same time this shows people are doing weird things in the init routine where they shouldn't be doing.

Also, having require do yield is generally very weird.. It does not seems to be reasonable use case. Even if we want to fix the crash, we should consider if this should even be supported in the first place.

@chronolaw chronolaw deleted the use-lua-require branch September 22, 2023 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author/community PRs from the open-source community (not Kong Inc) pr/discussion This PR is being debated. Probably just a few details. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants