You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/04/26 08:43:53 UTC
[GitHub] [pulsar] massakam opened a new pull request #10381: [Issue 10170] Optimize locks in AuthenticationAthenz
massakam opened a new pull request #10381:
URL: https://github.com/apache/pulsar/pull/10381
Fixes #10170
Currently, `AuthenticationAthenz` uses `synchronized` more than necessary. I think the only variables that can cause race conditions with multiple threads are `roleToken` and `cachedRoleTokenTimestamp`. Therefore, I introduced a locking object to prevent the race condition of these two variables, and removed the other `synchronized`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #10381: [Issue 10170] Optimize locks in AuthenticationAthenz
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #10381:
URL: https://github.com/apache/pulsar/pull/10381#discussion_r620442806
##########
File path: pulsar-client-auth-athenz/src/main/java/org/apache/pulsar/client/impl/auth/AuthenticationAthenz.java
##########
@@ -81,23 +82,24 @@ public String getAuthMethodName() {
}
@Override
- synchronized public AuthenticationDataProvider getAuthData() throws PulsarClientException {
- if (cachedRoleTokenIsValid()) {
- return new AuthenticationDataAthenz(roleToken, isNotBlank(roleHeader) ? roleHeader : ZTSClient.getHeader());
- }
- try {
- // the following would set up the API call that requests tokens from the server
- // that can only be used if they are 10 minutes from expiration and last twenty
- // four hours
- RoleToken token;
- synchronized (providerDomainLock) {
- token = getZtsClient().getRoleToken(providerDomain, null, minValidity, maxValidity, false);
+ public AuthenticationDataProvider getAuthData() throws PulsarClientException {
+ synchronized (cachedRoleTokenLock) {
Review comment:
This could actually be converted into a read-write lock, taking the read lock and upgrading to write lock to refresh the token
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #10381: [Issue 10170] Optimize locks in AuthenticationAthenz
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #10381:
URL: https://github.com/apache/pulsar/pull/10381
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] massakam commented on pull request #10381: [Issue 10170] Optimize locks in AuthenticationAthenz
Posted by GitBox <gi...@apache.org>.
massakam commented on pull request #10381:
URL: https://github.com/apache/pulsar/pull/10381#issuecomment-827347866
PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] massakam commented on a change in pull request #10381: [Issue 10170] Optimize locks in AuthenticationAthenz
Posted by GitBox <gi...@apache.org>.
massakam commented on a change in pull request #10381:
URL: https://github.com/apache/pulsar/pull/10381#discussion_r620898217
##########
File path: pulsar-client-auth-athenz/src/main/java/org/apache/pulsar/client/impl/auth/AuthenticationAthenz.java
##########
@@ -81,23 +82,24 @@ public String getAuthMethodName() {
}
@Override
- synchronized public AuthenticationDataProvider getAuthData() throws PulsarClientException {
- if (cachedRoleTokenIsValid()) {
- return new AuthenticationDataAthenz(roleToken, isNotBlank(roleHeader) ? roleHeader : ZTSClient.getHeader());
- }
- try {
- // the following would set up the API call that requests tokens from the server
- // that can only be used if they are 10 minutes from expiration and last twenty
- // four hours
- RoleToken token;
- synchronized (providerDomainLock) {
- token = getZtsClient().getRoleToken(providerDomain, null, minValidity, maxValidity, false);
+ public AuthenticationDataProvider getAuthData() throws PulsarClientException {
+ synchronized (cachedRoleTokenLock) {
Review comment:
@merlimat That's right. Introduced read-write lock.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org