-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
tls: append h2 to tlsconfig.NextProtos #2744
Conversation
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.
Is there any interaction between this PR and #2742?
return ps | ||
} | ||
} | ||
ret := make([]string, 0, len(ps)+1) |
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.
This seems like an unnecessary optimisation since the underlying array would at most end up as 4-10 elements in usual cases if the capacity was spent before an append. And this only happens when creating a new transport credential. Out of curiosity: is there something I'm missing?
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.
We can't modify the underlying array, as it's owned by the user, so it needs to be copied first. Since we know the final size of it, there's no reason not to allocate the right amount of memory up front.
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.
Thanks for the explanation dfawley, that makes a lot of sense!
I can confirm that this solves my problem in issue #2729 . It neatly block requests from a grpc client while it creates the certificate in the background and as soon as it is ready it completes the request. This works flawlessly with only port 443 opened. Hope this doesn't affect anything else and that we can get it into production soon :) |
fix #2729