support inline Lua code in per filter config #12516

wbpcode · 2020-08-06T11:11:42Z

Signed-off-by: wbpcode comems@msn.com

Commit Message:
Additional Description:
This is a supplement to PR #11235. Check #12460 for more info.

Risk Level: Low
Testing:
Docs Changes:
Release Notes:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

repokitteh-read-only · 2020-08-06T11:11:48Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #12516 was opened by wbpcode.

see: more, trace.

mattklein123

Thanks this is awesome. A few comments to get started.

/wait

source/extensions/filters/http/lua/config.cc

mattklein123 · 2020-08-06T16:59:13Z

source/extensions/filters/http/lua/lua_filter.h

-  PerLuaCodeSetup(const std::string& lua_code, ThreadLocal::SlotAllocator& tls);
+  PerLuaCodeSetup(const std::string& lua_code, ThreadLocal::SlotAllocator& tls,
+                  Event::Dispatcher& dispatcher);
+  ~PerLuaCodeSetup() { dispatcher_.deferredDelete(std::move(lua_state_)); }


What was the reason you added deferred delete here? I think this part is a little tricky unfortunately.

Basically, I think it's possible for the route table to get removed from the main thread but still be stored by an active request. Thus, the request may hold that last reference to the route table and then when it's over it will destroy the route table and the per-lua code. I think this means that this may happen cross thread. Is deferred delete safe cross thread? (It may be I would need to check.) Anyway this is the part that needs very careful thought. Can you describe what you did here and add more comments so we can discuss?

This 'dispatcher_' belongs to the main thread. And add this deferred delete will move TheadLocalState(lua_state_) and destroy it on the main thread.

In this case, no matter on which thread the route table is destroyed, we can guarantee that TheadLocalState(lua_state_) will only be destroyed in the main thread.

In this way, we can ensure that the state of TLS will not be modified on the worker thread.

I rechecked the source code and found that deferredDelete itself is not thread-safe, and it cannot safe cross thread.

Perhaps we can simply use post callback to let the main thread destroy TheadLocalState(lua_state_).
Just like following code:

~PerLuaCodeSetup() { dispatcher_.post([state = std::move(lua_state_)]() {}); // dispatcher_.deferredDelete(std::move(lua_state_)); }

It's possible this will work but the threading would need to be analyzed. Do you want to try this out and we can take a look?

Ok. I will update it this weekend. My own branch also needs to verify this function in the production environment immediately.

wbpcode · 2020-08-20T05:27:52Z

It took more time to complete this work than expected. Especially to resolve TSAN and ASAN alarms. Now, it can accept review. An integration test is added to verify thread safety issues in the case of route table updates. @mattklein123

dio · 2020-08-21T23:57:55Z

source/extensions/filters/http/lua/lua_filter.cc

-  if (lua_state_.getGlobalRef(request_function_slot_) == LUA_REFNIL) {
+PerLuaCodeSetup::PerLuaCodeSetup(const std::string& lua_code, ThreadLocal::SlotAllocator& tls,
+                                 Event::Dispatcher& dispatcher)
+    : lua_state_(new Filters::Common::Lua::ThreadLocalState{lua_code, tls}),


A question, for my own education, why do we need to have lua_state_ as a pointer here? Since this will be still owned by this class right?

For thread safety, ThreadLocalState can only be destructured in the main thread. However, when the route table is dynamically updated, the old route configuration may be destroyed in a random worker thread. Its PerLuaCodeSetup will also be destructured in the corresponding worker thread. By converting lua_state_ to a pointer, we can push the destruction of lua_state_ into the main thread with a callback during PerLuaCodeSetup destructuring.

virtual ~PerLuaCodeSetup() { if (dispatcher_.isThreadSafe()) { return; } dispatcher_.post([state = lua_state_.release()] { delete state; }); }

Aha! I have a new idea. Thank you very much for this question @dio . It reminded me that there's actually a different way to think about making the code a little cleaner.

When this is ready for review can you remove [WIP] from the title also? Thank you.

/wait

All right. 🆗

Signed-off-by: wbpcode <comems@msn.com>

wbpcode · 2020-08-25T07:10:51Z

It's ready for a review. Thanks. @mattklein123 @dio

mattklein123

Thanks this is fantastic. Nicely done. Just a few comments.

/wait

mattklein123 · 2020-08-26T17:31:03Z

source/extensions/filters/common/lua/lua.cc

+    // lua_State, some static variables are used to assist memory allocation. Creating lua_State in
+    // different threads at the same time may cause data race. Although 'LuaJIT' uses retries to
+    // ensure the final safety of the memory allocation, it may still trigger the TSAN alarm.
+    static Thread::MutexBasicLockable lua_thread_local_lock;


potential static fiasco here. Please use CONSTRUCT_ON_FIRST_USE

mattklein123 · 2020-08-26T17:35:21Z

source/extensions/filters/http/lua/lua_filter.h

+
+  ~FilterConfigPerRoute() override {
+    if (per_lua_code_setup_ptr_ && !main_thread_dispatcher_.isThreadSafe()) {
+      main_thread_dispatcher_.post([setup = per_lua_code_setup_ptr_.release()] { delete setup; });


I agree this is what is needed, but can you add more comments on why this is necessary? Technically I think it's possible this might leak during shutdown but it probably doesn't matter. If you want to avoid a leak no matter what, create a shared_ptr wrapper, put that in the lambda, and then move the unique_ptr inside of it.

Thanks. I will add some short comment to explain the necessity and try to fix the possible memory leak.

api/envoy/extensions/filters/http/lua/v3/lua.proto

Signed-off-by: wbpcode <comems@msn.com>

mattklein123 · 2020-08-31T17:57:10Z

Filed #12896 for the CI issue. Sorry I think we have a unicode issue that needs to be fixed.

/wait

mattklein123 · 2020-08-31T20:55:56Z

Can you merge main? CI should be fixed. Thank you @asraa!

mattklein123

Thanks looks great. One small comment/question for discussion.

/wait-any

mattklein123 · 2020-09-01T18:22:31Z

source/extensions/filters/common/lua/lua.cc

+    // different threads at the same time may cause data race. Although 'LuaJIT' uses retries to
+    // ensure the final safety of the memory allocation, it may still trigger the TSAN alarm.
+    Thread::LockGuard guard(luaThreadLocalLock());


If we want to avoid the lock here, should we just disable the relevant tests under TSAN with a comment? I'm fine either way. cc @dio @lizan

If we want to avoid the lock here, should we just disable the relevant tests under TSAN with a comment? I'm fine either way. cc @dio @lizan

Yes. Only one test need to be disabled under TSAN. I think both would be acceptable, depending on what the community has to say.

I'm good with that.

Yeah let's just do that. Please have a good comment on why we are disabling TSAN for the test.

/wait

Signed-off-by: wbpcode <comems@msn.com>

mattklein123

Awesome work!

repokitteh-read-only bot added the api label Aug 6, 2020

mattklein123 assigned mattklein123 and dio Aug 6, 2020

mattklein123 requested changes Aug 6, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 6, 2020

repokitteh-read-only bot removed the waiting label Aug 17, 2020

mattklein123 added the waiting label Aug 17, 2020

repokitteh-read-only bot removed the waiting label Aug 18, 2020

dio reviewed Aug 21, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 22, 2020

support inline Lua code in per filter config(refactor)

1cdb3a3

Signed-off-by: wbpcode <comems@msn.com>

repokitteh-read-only bot removed the waiting label Aug 24, 2020

wbpcode force-pushed the route-lua branch from f40afe8 to 1cdb3a3 Compare August 24, 2020 13:07

mattklein123 added the waiting label Aug 24, 2020

fix clang tidy

85060a1

Signed-off-by: wbpcode <comems@msn.com>

repokitteh-read-only bot removed the waiting label Aug 25, 2020

wbpcode added 2 commits August 25, 2020 11:00

Kick CI

4e3ad3a

Signed-off-by: wbpcode <comems@msn.com>

Merge branch 'master' of github.com:envoyproxy/envoy into route-lua

6510ffb

wbpcode changed the title ~~[WIP] support inline Lua code in per filter config~~ support inline Lua code in per filter config Aug 25, 2020

mattklein123 requested changes Aug 26, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 26, 2020

wbpcode added 2 commits August 28, 2020 09:51

Merge branch 'master' of github.com:envoyproxy/envoy into route-lua

8249530

add comment & Fixed possible memory leak

12464da

Signed-off-by: wbpcode <comems@msn.com>

repokitteh-read-only bot removed the waiting label Aug 31, 2020

wbpcode added 2 commits August 31, 2020 15:15

Merge branch 'master' of github.com:envoyproxy/envoy into route-lua

755c8b4

Kick CI

47d978b

Signed-off-by: wbpcode <comems@msn.com>

mattklein123 mentioned this pull request Aug 31, 2020

flaky tests: unicode processing bug #12896

Closed

repokitteh-read-only bot added the waiting label Aug 31, 2020

asraa mentioned this pull request Aug 31, 2020

[TEST CI] Test flaky_test encoding #12900

Closed

Merge branch 'master' of github.com:envoyproxy/envoy into route-lua

c158b26

repokitteh-read-only bot removed the waiting label Sep 1, 2020

mattklein123 reviewed Sep 1, 2020

View reviewed changes

repokitteh-read-only bot added waiting:any waiting and removed waiting:any labels Sep 1, 2020

remote lock and disable tsan

a938333

Signed-off-by: wbpcode <comems@msn.com>

repokitteh-read-only bot removed the waiting label Sep 3, 2020

mattklein123 approved these changes Sep 3, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Sep 3, 2020

mattklein123 merged commit 47bd839 into envoyproxy:master Sep 3, 2020

wbpcode mentioned this pull request Jul 12, 2022

lua: deprecate inline_code and use default_source_code #22125

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support inline Lua code in per filter config #12516

support inline Lua code in per filter config #12516

wbpcode commented Aug 6, 2020 •

edited

Loading

repokitteh-read-only bot commented Aug 6, 2020

mattklein123 left a comment

mattklein123 Aug 6, 2020

wbpcode Aug 7, 2020 •

edited

Loading

wbpcode Aug 7, 2020 •

edited

Loading

mattklein123 Aug 10, 2020

wbpcode Aug 11, 2020

wbpcode commented Aug 20, 2020

dio Aug 21, 2020 •

edited

Loading

wbpcode Aug 22, 2020

wbpcode Aug 22, 2020 •

edited

Loading

mattklein123 Aug 22, 2020

wbpcode Aug 25, 2020 •

edited

Loading

wbpcode commented Aug 25, 2020 •

edited

Loading

mattklein123 left a comment

mattklein123 Aug 26, 2020

mattklein123 Aug 26, 2020

wbpcode Aug 31, 2020

mattklein123 commented Aug 31, 2020

mattklein123 commented Aug 31, 2020

mattklein123 left a comment

mattklein123 Sep 1, 2020

wbpcode Sep 2, 2020

dio Sep 2, 2020

mattklein123 Sep 2, 2020

wbpcode Sep 3, 2020

mattklein123 left a comment

support inline Lua code in per filter config #12516

support inline Lua code in per filter config #12516

Conversation

wbpcode commented Aug 6, 2020 • edited Loading

repokitteh-read-only bot commented Aug 6, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

wbpcode Aug 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode commented Aug 20, 2020

dio Aug 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode Aug 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

wbpcode commented Aug 25, 2020 • edited Loading

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Aug 31, 2020

mattklein123 commented Aug 31, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

wbpcode commented Aug 6, 2020 •

edited

Loading

wbpcode Aug 7, 2020 •

edited

Loading

wbpcode Aug 7, 2020 •

edited

Loading

dio Aug 21, 2020 •

edited

Loading

wbpcode Aug 22, 2020 •

edited

Loading

wbpcode Aug 25, 2020 •

edited

Loading

wbpcode commented Aug 25, 2020 •

edited

Loading