Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
dynamic_modules: adds send response #37856
base: main
Are you sure you want to change the base?
dynamic_modules: adds send response #37856
Changes from 12 commits
f16c3a6
2cf5cc6
45971ea
c86821f
86fe96a
304c395
e59e224
c12d800
bfb49f9
006082d
3ad32e4
b8a7ef1
f94fb8b
d56ac0e
db1d1d2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I doubt independent lifetimes here are actually needed. The trait impl below doesn't even repeat them either (there is a clippy lint that usually fires about this, maybe we don't have it on.) I would just remove them all. The default lifetime should be ok I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without them it fails with
AFAICT, it's not possible due with mockall. asomers/mockall#61
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, at the top of the trait, we explicitly disable
clippy::needless_lifetimes
because of this:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very, very surprised this works. I don't really see how it can so I must be missing something. Is this covered by a test? I don't see what could properly convert this into the struct form that the ABI expects? If it does work I would be concerned that it's basically by accident. I'm not sure if layout guarantees this will always be true. Maybe it does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is an (unstable) secret that I have used in another project... basically Rust hasn't changed the memory layout of &[u8] (and &str which is exactly the same as the former at memory level) before since the release v1.0 which is a pair of (ptr and size). Also it hasn't changed the layout of tuple in the sense that it always packs in the order in which element appear. That's the reason why this works. Having said that though, this is also not guaranteed by Rust team whose ABI is not stable.
We can copy the vector here into envoy_dynamic_module_type_module_http_header with the stable repr(C) guaranteed by the Rust compiler to ensure the stability but not sure if we want to do that or not. On one hand, I think it should be fine and detected by the CI when the new Rust toolchain breaks this, which hans't happened before at all, but on the other hand, we can eliminate the concern over the (negligible?) gain.
Alternatively, we can accept the iterator at this API that allows us to construct the C stable vector without constructing vectors twice (by user and internally here with the current code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did it crash without this? I haven't been in the Envoy code deeply in a long time but historically I'm pretty sure that once a local reply is sent the filter should no longer be called again as everything will be reset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
back in the day i remember this crashes the wasm plugins because of the reentrance causing the multiple mutable borrowing, and that's why I suggested to do this - but i think it's worth revisiting this and see if this actually crashes... maybe the envoy behavior has changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able to come up with a crasher (but that doesn't mean it's not possible).
The similar guard in the wasm filter was added here: #16593
But since that PR merged, b361fb6#diff-e91327a311d8130daac76a7c43dca2ae70d5a6a37dc7fdb658cf25b2fde88cc9R534 and #24367 were added that seems like it would make the filter callbacks non-reentrant after a local reply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hah i didn't know that. I think let's remove this flag then, and fix it if someone encounters the problem