Thread safe improvements on OIDCDiscoveryConfigListener #4159
Conversation
knative-prow
bot
added
size/L
knative-prow
bot
added
the
approved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4159 +/- ##
=======================================
Coverage 45.51% 45.51%
=======================================
Files 270 270
Lines 19925 19925
=======================================
Hits 9069 9069
Misses 10129 10129
Partials 727 727 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Matthias Wessendorf <[email protected]>
knative-prow
bot
added
size/S
...n/java/dev/knative/eventing/kafka/broker/receiver/impl/auth/OIDCDiscoveryConfigListener.java
Show resolved
Hide resolved
| .forEach(c -> c.accept(this.oidcDiscoveryConfig)); | ||
| OIDCDiscoveryConfig config = this.oidcDiscoveryConfig.get(); | ||
| if (config != null) { | ||
| this.callbacks.forEach(callback -> callback.accept(config)); |
There was a problem hiding this comment.
I see in deregisterCallback we set callback (the element) to null, don't we need to check that callback is not null before calling accept ?
knative-prow
bot
added
size/M
| this.callbacks = new ArrayList<>(); | ||
| } | ||
|
|
||
| public synchronized int registerCallback(Consumer<OIDCDiscoveryConfig> callback) { |
There was a problem hiding this comment.
One other issue I see with index based registration is that with addition and removal of callbacks the array is ever growing (and therefore a memory leak), which can lead to OOM, it's better to have a map id -> callback so that we can remove elements during registration rather than setting them to null
There was a problem hiding this comment.
you mean like ConcurrentMap ? instead of the CopyOnWriteArrayList?
Signed-off-by: Matthias Wessendorf <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: matzew, pierDipi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cherry-pick release-1.16 |
|
@matzew: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cherry-pick release-1.15 |
|
@matzew: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@matzew: new pull request created: #4160 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@matzew: new pull request created: #4161 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Proposed Changes
ConcurrentHashMap,AtomicRefandAtomicIntfor improving thread safenessRelease Note
Docs