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/02/25 06:33:26 UTC
[GitHub] [pulsar] lhotari opened a new issue #9711: Creating and modifying authorization policies in not thread safe
lhotari opened a new issue #9711:
URL: https://github.com/apache/pulsar/issues/9711
**Describe the bug**
Creating and modifying authorization policies is not thread safe. It leads to unexpected NullPointerExceptions for example and could lead to authorization data corruption.
**To Reproduce**
The flaky test #9709 is one example of the problem.
**Expected behavior**
Creating and modifying authorization policies should be thread safe.
**Observations**
It looks like the [org.apache.pulsar.common.policies.data.AuthPolicies](https://github.com/apache/pulsar/blob/master/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/AuthPolicies.java) data structures are modified without a way to ensure thread safety and data consistency.
----------------------------------------------------------------
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] lhotari removed a comment on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
lhotari removed a comment on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-787677910
While investigating the flakiness in ReplicatorTest, I happened to come across this NPE related to the issues in creating and modifying authorization policies:
```
Caused by: java.lang.NullPointerException
at java.util.TreeMap.getEntry(TreeMap.java:347) ~[?:1.8.0_275]
at java.util.TreeMap.get(TreeMap.java:278) ~[?:1.8.0_275]
at org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider.lambda$checkPermission$12(PulsarAuthorizationProvider.java:404) ~[pulsar-broker-common-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
at java.util.concurrent.CompletableFuture.uniAccept(CompletableFuture.java:670) ~[?:1.8.0_275]
at java.util.concurrent.CompletableFuture.uniAcceptStage(CompletableFuture.java:683) ~[?:1.8.0_275]
at java.util.concurrent.CompletableFuture.thenAccept(CompletableFuture.java:2010) ~[?:1.8.0_275]
at org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider.checkPermission(PulsarAuthorizationProvider.java:397) ~[pulsar-broker-common-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
at org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider.checkAuthorization(PulsarAuthorizationProvider.java:378) ~[pulsar-broker-common-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
at org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider.lambda$canConsumeAsync$2(PulsarAuthorizationProvider.java:148) ~[pulsar-broker-common-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
at java.util.concurrent.CompletableFuture.uniAccept(CompletableFuture.java:670) ~[?:1.8.0_275]
at java.util.concurrent.CompletableFuture.uniAcceptStage(CompletableFuture.java:683) ~[?:1.8.0_275]
at java.util.concurrent.CompletableFuture.thenAccept(CompletableFuture.java:2010) ~[?:1.8.0_275]
at org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider.canConsumeAsync(PulsarAuthorizationProvider.java:112) ~[pulsar-broker-common-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
at org.apache.pulsar.broker.authorization.AuthorizationService.lambda$canConsumeAsync$1(AuthorizationService.java:216) ~[pulsar-broker-common-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
at java.util.concurrent.CompletableFuture.uniCompose(CompletableFuture.java:966) ~[?:1.8.0_275]
at java.util.concurrent.CompletableFuture$UniCompose.tryFire(CompletableFuture.java:940) ~[?:1.8.0_275]
at java.util.concurrent.CompletableFuture$Completion.run(CompletableFuture.java:456) ~[?:1.8.0_275]
... 1 more
```
----------------------------------------------------------------
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] lhotari edited a comment on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-786893444
> Could you give an example like the following?
the problem is not about certain sequence of events or interactions with zk. It's about unsafe mutation and access of the TreeMap / HashMap instances contained in the AuthPolicies class. A TreeMap or HashMap can only be safely mutated when all access (mutation and reading) happens under the same object monitor. Violating this rule will lead to nondeterministic behavior.
----------------------------------------------------------------
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] 315157973 commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
315157973 commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-792623726
> New objects are only a problem if it causes a bottleneck or excessive GC pressure. I don't think that policy modifications would be a large part of the workload. Therefore there isn't a reason to reject solutions around immutability.
In the scene of this issue, compared with ConcurrentHashMap, what are its advantages?
----------------------------------------------------------------
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] lhotari commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
lhotari commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-792520398
On a busy system, unsafe HashMap mutation and access will eventually lead to infinite loops and 100% CPU core usage when the HashMap data structures get corrupted.
This problem is [demonstrated and explained in this article](https://dzone.com/articles/java-7-hashmap-vs), in the section "HashMap infinite looping problem replication".
I might be missing something, but I can see that the Pulsar policy creation and modification contains quite a few locations where there is unsafe HashMap/TreeMap mutation and access. @merlimat 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] 315157973 commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
315157973 commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-792550726
@lhotari How about change HashMap to ConcurrentHashMap, and do not update the cache, only invalidate the cache
----------------------------------------------------------------
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] lhotari commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
lhotari commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-787730434
It seems that ReplicatorTest triggers some of the thread safety issues in updating policies. Some of the [reported issues about ReplicatorTest flakiness](https://github.com/apache/pulsar/search?o=desc&q=replicatortest&s=updated&type=issues).
----------------------------------------------------------------
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] lhotari removed a comment on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
lhotari removed a comment on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-786763770
Another example of missing concurrency control:
https://github.com/apache/pulsar/blob/1fab5aa69446b4c2eaebb615d35fe9f5a509ba4c/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java#L325-L330
----------------------------------------------------------------
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] lhotari edited a comment on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-792620618
> Creating new objects will produce garbage, I think ConcurrentHashMap will be better
New objects are only a problem if it causes a bottleneck or excessive GC pressure. I don't think that policy modifications would be a large part of the workload. Therefore there isn't a reason to reject solutions around immutability.
btw. Persistent data collections such as [PCollections library](https://github.com/hrldcpr/pcollections) help minimize the number of new objects that are created when using immutable collections.
----------------------------------------------------------------
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] eolivelli commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-785678831
Good catch @lhotari
I believe this is a blocker issue.
@wolfstudy @codelipenghui can we fix it for 2.7.1 ?
----------------------------------------------------------------
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] lhotari commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
lhotari commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-792592818
> @lhotari How about change HashMap to ConcurrentHashMap, and do not update the cache, only invalidate the cache .
+1, I guess ConcurrentHashMap is one possibility and probably leads to the simplest fix to the issue.
In general, I'd prefer a solution that is based on immutability so that an update returns a new copy and new instance. This approach is used in an open PR #9598 . However in that case, there aren't Maps that are involved.
Using persistent collections such as [PCollections library](https://github.com/hrldcpr/pcollections) could be useful in the immutable approach.
----------------------------------------------------------------
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] 315157973 commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
315157973 commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-786391002
When updating, a version will be passed in to determine whether the data has been modified. This is an optimistic lock. Why is there a thread safety issue?
----------------------------------------------------------------
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] 315157973 removed a comment on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
315157973 removed a comment on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-792550726
@lhotari How about change HashMap to ConcurrentHashMap, and do not update the cache, only invalidate the cache .
The cache has a timeout period, which can minimize the problem of cache inconsistency.
----------------------------------------------------------------
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] 315157973 edited a comment on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
315157973 edited a comment on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-792550726
@lhotari How about change HashMap to ConcurrentHashMap, and do not update the cache, only invalidate the cache .
The cache has a timeout period, which can minimize the problem of cache inconsistency.
----------------------------------------------------------------
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] lhotari commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
lhotari commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-787677910
While investigating the flakiness in ReplicatorTest, I happened to come across this NPE related to the issues in creating and modifying authorization policies:
```
Caused by: java.lang.NullPointerException
at java.util.TreeMap.getEntry(TreeMap.java:347) ~[?:1.8.0_275]
at java.util.TreeMap.get(TreeMap.java:278) ~[?:1.8.0_275]
at org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider.lambda$checkPermission$12(PulsarAuthorizationProvider.java:404) ~[pulsar-broker-common-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
at java.util.concurrent.CompletableFuture.uniAccept(CompletableFuture.java:670) ~[?:1.8.0_275]
at java.util.concurrent.CompletableFuture.uniAcceptStage(CompletableFuture.java:683) ~[?:1.8.0_275]
at java.util.concurrent.CompletableFuture.thenAccept(CompletableFuture.java:2010) ~[?:1.8.0_275]
at org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider.checkPermission(PulsarAuthorizationProvider.java:397) ~[pulsar-broker-common-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
at org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider.checkAuthorization(PulsarAuthorizationProvider.java:378) ~[pulsar-broker-common-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
at org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider.lambda$canConsumeAsync$2(PulsarAuthorizationProvider.java:148) ~[pulsar-broker-common-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
at java.util.concurrent.CompletableFuture.uniAccept(CompletableFuture.java:670) ~[?:1.8.0_275]
at java.util.concurrent.CompletableFuture.uniAcceptStage(CompletableFuture.java:683) ~[?:1.8.0_275]
at java.util.concurrent.CompletableFuture.thenAccept(CompletableFuture.java:2010) ~[?:1.8.0_275]
at org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider.canConsumeAsync(PulsarAuthorizationProvider.java:112) ~[pulsar-broker-common-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
at org.apache.pulsar.broker.authorization.AuthorizationService.lambda$canConsumeAsync$1(AuthorizationService.java:216) ~[pulsar-broker-common-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
at java.util.concurrent.CompletableFuture.uniCompose(CompletableFuture.java:966) ~[?:1.8.0_275]
at java.util.concurrent.CompletableFuture$UniCompose.tryFire(CompletableFuture.java:940) ~[?:1.8.0_275]
at java.util.concurrent.CompletableFuture$Completion.run(CompletableFuture.java:456) ~[?:1.8.0_275]
... 1 more
```
----------------------------------------------------------------
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] codelipenghui commented on issue #9711: Creating and modifying authorization policies is not thread safe
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-1058893000
The issue had no activity for 30 days, mark with Stale label.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
lhotari commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-793770462
> In the scene of this issue, compared with ConcurrentHashMap, what are its advantages?
One of the advantages of immutable objects (all fields final in Java) is that there isn't any need for locks at access time after there is a reference to the object. A design where there's some kind of "single writer principle" and immutable objects can be easier to get right. However it might be an overkill to modify an existing solution to follow this type of pattern.
I guess that the benefit of using ConcurrentHashMap (or Collections.synchronizedMap wrapper) is that the change is relatively small to the existing solution as you have already demonstrated in the PR #9850 .
----------------------------------------------------------------
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] codelipenghui commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-789028532
@eolivelli we don't want to block the 2.7.1 release, instead we can accelerate the 2.7.2 release. Here is the release proposal that we have discussed before https://github.com/apache/pulsar/wiki/PIP-47%3A-Time-Based-Release-Plan. If the problem is not introduced after 2.7.0 and it does not break the existing behavior, it's better to do not to block the 2.7.1 release.
----------------------------------------------------------------
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] lhotari commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
lhotari commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-792620618
> Creating new objects will produce garbage, I think ConcurrentHashMap will be better
New objects are only a problem if it causes a bottleneck. I don't think that policy modifications would be a large part of the workload. Therefore there isn't a reason to reject solutions around immutability.
btw. Persistent data collections such as [PCollections library](https://github.com/hrldcpr/pcollections) help minimize the number of new objects that are created when using immutable collections.
----------------------------------------------------------------
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] eolivelli commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-789147963
@codelipenghui I am fine with keeping it only on master branch.
----------------------------------------------------------------
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] 315157973 commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
315157973 commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-786783641
> > When updating, a version will be passed in to determine whether the data has been modified. This is an optimistic lock. Why is there a thread safety issue?
>
> that part is not the concern. The concerns are the maps that are contained in the cached AuthPolicies entry. They are mutated without synchronization on both read & write access. Mutation for example here:
> https://github.com/apache/pulsar/blob/7001f64aab6335b5192b408dba1e85602dd5ea6b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java#L688
>
>
> Accessed here:
> https://github.com/apache/pulsar/blob/1fab5aa69446b4c2eaebb615d35fe9f5a509ba4c/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java#L206
Could you give an example like the following?
```
//MetadataCacheImpl.java 175
return store.put(path, newValue, Optional.of(expectedVersion)).thenAccept(stat -> {
objCache.put(path,
FutureUtils.value(Optional.of(new SimpleImmutableEntry<T, Stat>(newValueObj, stat))));
});
```
Normal scene:
t1: thread 1 updates zk successfully
t2: thread 1 updates the cache
t3: thread 2 updates zk successfully
t4: Thread 2 updates the cache
Concurrent scene:
t1: thread 1 updates zk successfully
t2: Thread 2 updates zk, which fails because the version is different
t3: Thread 1 updates the cache
----------------------------------------------------------------
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] 315157973 commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
315157973 commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-792613608
> In general, I'd prefer a solution that is based on immutability so that an update returns a new copy and new instance. This approach is used in an open PR #9598 . However in that case, there aren't Maps that are involved.
Creating new objects will produce garbage, I think ConcurrentHashMap will be better
----------------------------------------------------------------
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] lhotari commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
lhotari commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-786893444
> Could you give an example like the following?
the problem is not about certain sequence of events. It's about unsafe mutation and access of the TreeMap / HashMap instances contained in the AuthPolicies class. A TreeMap or HashMap can only be safely mutated when all access (mutation and reading) happens under the same object monitor. Violating this rule will lead to undeterministic behavior.
----------------------------------------------------------------
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] lhotari edited a comment on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-786631166
> When updating, a version will be passed in to determine whether the data has been modified. This is an optimistic lock. Why is there a thread safety issue?
that part is not the concern. The concerns are the maps that are contained in the cached AuthPolicies entry. They are mutated without synchronization on both read & write access. Mutation for example here:
https://github.com/apache/pulsar/blob/7001f64aab6335b5192b408dba1e85602dd5ea6b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java#L688
Accessed here:
https://github.com/apache/pulsar/blob/1fab5aa69446b4c2eaebb615d35fe9f5a509ba4c/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java#L206
----------------------------------------------------------------
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] lhotari edited a comment on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-786893444
> Could you give an example like the following?
the problem is not about certain sequence of events or interactions with zk. It's about unsafe mutation and access of the TreeMap / HashMap instances contained in the AuthPolicies class. A TreeMap or HashMap can only be safely mutated when all access (mutation and reading) happens under the same object monitor. Violating this rule will lead to undeterministic behavior.
----------------------------------------------------------------
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] lhotari commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
lhotari commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-786763770
Another example of missing concurrency control:
https://github.com/apache/pulsar/blob/1fab5aa69446b4c2eaebb615d35fe9f5a509ba4c/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java#L325-L330
----------------------------------------------------------------
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] lhotari commented on issue #9711: Creating and modifying authorization policies in not thread safe
Posted by GitBox <gi...@apache.org>.
lhotari commented on issue #9711:
URL: https://github.com/apache/pulsar/issues/9711#issuecomment-786631166
> When updating, a version will be passed in to determine whether the data has been modified. This is an optimistic lock. Why is there a thread safety issue?
that part is not the concern. The concerns are the maps that are the maps contained in the cached AuthPolicies entry. They are mutated without synchronization on both read & write access. Mutation for example here:
https://github.com/apache/pulsar/blob/7001f64aab6335b5192b408dba1e85602dd5ea6b/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java#L688
Accessed here:
https://github.com/apache/pulsar/blob/1fab5aa69446b4c2eaebb615d35fe9f5a509ba4c/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java#L206
----------------------------------------------------------------
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