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

wasm: use in_vm_context_created_ flag for http contexts. #16202

Merged
merged 1 commit into from
Apr 28, 2021

Conversation

mathetake
Copy link
Member

@mathetake mathetake commented Apr 28, 2021

Removed http_request_started_ and instead use the in_vm_context_created_ in Proxy-Wasm C++ host since it has exactly the same meaning (indicating whether onCreate is called or not). Network filter is already using in_vm_context_created_.

Signed-off-by: Takeshi Yoneda [email protected]

Copy link
Contributor

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

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

Removed http_request_started_ and instead use the in_vm_context_created_ in Proxy-Wasm C++ host since it has exactly the same meaning (indicating whether onCreate is called or not). Network filter is already using in_vm_context_created_.

It indicates that HTTP request was started, not that onCreate() was called, which is not necessairly the same thing, since the latter can happen without active HTTP request.

Note that this was originally in_vm_context_created_ (envoyproxy/envoy-wasm#521), but it was later changed to http_request_started_ (envoyproxy/envoy-wasm#538). Unfortunately, it's not clear from that PR why the change happened, since it's mixed with other changes and added as part of "Update tests" review commit (envoyproxy/envoy-wasm@6c08032).

Having said that, I think it's fine to merge this. Worst case, we can revert it and add regression tests if anything breaks.

@mathetake
Copy link
Member Author

Thanks for digging into the history!

Having said that, I think it's fine to merge this. Worst case, we can revert it and add regression tests if anything breaks.

Yeah, let's see how it goes.

@yanavlasov yanavlasov merged commit c75ac18 into envoyproxy:main Apr 28, 2021
@mathetake mathetake deleted the wasm-refactor branch April 28, 2021 14:52
gokulnair pushed a commit to gokulnair/envoy that referenced this pull request May 6, 2021
…16202)

Removed http_request_started_ and instead use the in_vm_context_created_ in Proxy-Wasm C++ host since it has exactly the same meaning (indicating whether onCreate is called or not). Network filter is already using in_vm_context_created_.

Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Gokul Nair <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants