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

Listeners > BootSSLContext: improve locking access to cached sslConte… #295

Merged
merged 3 commits into from
Jun 7, 2021

Conversation

pv3ntur1
Copy link
Contributor

@pv3ntur1 pv3ntur1 commented Jun 7, 2021

PR for #294

@pv3ntur1 pv3ntur1 requested a review from nicoloboschi June 7, 2021 09:11
Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

It is promising, but you are missing local map invalidation

LOG.log(Level.FINER, "resolve SNI mapping " + sniHostname + ", key: " + key);
private final class ListenerSniMapping implements io.netty.util.AsyncMapping<String, SslContext> {

private final Map<String, SslContext> listenerSslContexts = new ConcurrentHashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to be Concurrent as is accessed only by this thread

return bootSslContext(listener, choosen);
} catch (ConfigurationNotValidException err) {
throw new RuntimeException(err);
sslContext = bootSslContext(listener, choosen);
Copy link
Contributor

Choose a reason for hiding this comment

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

idk if you did it on purpose, but now there is a possibility where more than one listener will create the SslContext for the same key
btw this is a less locking-situation so I hope this will decrease latency (maybe with more CPU usages in some cases)

throw new RuntimeException(err);
sslContext = bootSslContext(listener, choosen);
listenerSslContexts.put(key, sslContext);
sslContexts.put(key, sslContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

sslContexts is invalidated when the config changes, we need to invalidate this local map as well

Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

+1

@nicoloboschi nicoloboschi merged commit e61dcdb into master Jun 7, 2021
pv3ntur1 added a commit that referenced this pull request Jun 8, 2021
#295)

* Listeners > BootSSLContext: improve locking access to cached sslContexes #294

* fix pr comments

* fix cache clear

Co-authored-by: Paolo Venturi <[email protected]>
@pv3ntur1 pv3ntur1 deleted the improvement/#294 branch August 31, 2021 12:35
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.

2 participants