Skip to content

feature: use the actual ngx_connection_t * in ssl_client_hello_by #2416

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

Conversation

climagabriel
Copy link
Contributor

@climagabriel climagabriel commented May 8, 2025

Do we really need to create a fake connection here? Sure, the request struct wasn't even created yet but the conneciton does exist.
I personally need it because I need to allocate memory that will live for as long as the connection exists, not until the Lua chunk returns.

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

image

Also, surely there must be a better way to do this.
c = ngx_ssl_get_connection(ssl_conn);
This line already does exist at the very top of ngx_http_lua_ssl_client_hello_handler().

If the real connection exists, can't we make it available in the Lua code itself?
The same way we make the fake request available by resty.core.base.get_request

Do we really need to create a fake connection here?
Sure, the request struct wasn't even created yet but the conneciton does
exist.
I personally need it because I need to allocate memory that will live
for as long as the connection exists, not until the Lua chunk returns.
zhuizhuhaomeng
zhuizhuhaomeng previously approved these changes May 8, 2025
@climagabriel
Copy link
Contributor Author

climagabriel commented May 9, 2025

I read a bit more about openresty and realized that data persisting beyond the lifetime of a request goes against the design principles of the module so I'm closing this PR.

@climagabriel climagabriel deleted the feature_connection_pointer_in_ssl_client_hello_by branch May 9, 2025 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants