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/05/04 12:32:34 UTC
[GitHub] [pulsar] lhotari opened a new pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
lhotari opened a new pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480
Fixes #10433
### Motivation
See #10433 . There's a rare race condition in invalidating ledger cache entries stored in RangeCache .
### Modifications
- add separate `invalidate` method for invalidating `EntryImpl` ledger cache entries. This prevents race conditions in invalidation.
--
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 a change in pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#discussion_r627400020
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/EntryImpl.java
##########
@@ -166,7 +170,16 @@ protected void deallocate() {
timestamp = -1;
ledgerId = -1;
entryId = -1;
+ invalidated.set(false);
recyclerHandle.recycle(this);
}
+ @Override
+ public boolean invalidate() {
+ if (invalidated.compareAndSet(false, true)) {
+ release();
+ return true;
+ }
+ return false;
Review comment:
> This will avoid decrementing the ref-count more than once, but it will not prevent the 2nd thread from accessing an entry whose ref-count was already to 0.
Yes, that's a good point. I'm thinking of a solution where invalidation would be a completely separate operation which triggers when reference count is 1 or gets back to 1. Another protection here would be a change in logic that release operations to change reference count from 1 to 0 would be rejected completely. That would prevent bugs which are caused by release being called too many times. Those issues could be logged and fixed if such bugs exist.
--
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 pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#issuecomment-833502229
I've been trying to spot locations where the entry could get released multiple times, but still continued to be used.
This looks risky:
https://github.com/apache/pulsar/blob/dcaa1d350fec920ef0ba32b4aeecd2e9604d4824/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/EntryCacheImpl.java#L191-L195
Together with
https://github.com/apache/pulsar/blob/d2138f7c4a84bc4d8b1d07600af15b30b7e2416e/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L1232-L1237
--
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 closed pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
lhotari closed pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480
--
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 pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#issuecomment-832604122
> @lhotari I agree with @merlimat on " If the issue is that we're using an already released buffer, we should fix that instead."
Yes I agree on this, I'll try to dig deeper. :)
> It could be caused by i.e. an implementation of the ReadEntriesCallback/ReadEntryCallback/whatever other callback that is expected to properly release entry.
Good point. I'll track those release calls.
> Or, as you mentioned at the issue, it could be [netty/netty#10986](https://github.com/netty/netty/issues/10986)
Pulsar might not be impacted in cases where AbstractCASReferenceCounted base class is used. Here's AbstractCASReferenceCounted release logic:
https://github.com/apache/pulsar/blob/5e446a67fcacf340d352eeafdce15a78fb971f19/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/AbstractCASReferenceCounted.java#L95-L110
>
> Other issue is: do we have a repro?
No. I assume that it's a rare issue since there's not many reports about it. It could be possible to achieve a repro at some kind of unit/integration test level using [JCStress](https://github.com/openjdk/jcstress), but it's hard to estimate the effort to achieve a repro.
> Have we confirmed that upgrade of Netty to a version with fix [netty/netty#10986](https://github.com/netty/netty/issues/10986) doesn't help?
No. The bug report is about concurrent calls to `.recycle()`. As mentioned earlier, that is prevented with AbstractCASReferenceCounted in Pulsar in most cases.
--
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 #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#discussion_r625920033
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/RangeCache.java
##########
@@ -140,9 +140,11 @@ public Value get(Key key) {
continue;
}
- removedSize += weighter.getSize(value);
- value.release();
- ++removedEntries;
+ long entrySize = weighter.getSize(value);
Review comment:
eg: in case of concurrent invalidate, the value is already invalid here
--
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 a change in pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#discussion_r626396797
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/RangeCache.java
##########
@@ -90,7 +90,7 @@ public Value get(Key key) {
try {
value.retain();
return value;
- } catch (Throwable t) {
+ } catch (IllegalReferenceCountException e) {
Review comment:
yes, logging would be useful to get more information. I'm just wondering if it should be done only at debug level since it's not a real problem. It's part of the expected behavior that this could sometimes happen.
--
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 closed pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
lhotari closed pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480
--
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 pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#issuecomment-832445354
> Actually, I'm not 100% sure that the having invalidations called by multiple thread could lead to the issue. In all the cases the entries are removed from the `ConcurrentSkipList` before getting release, and the guarantee there is that the removal should be atomic.
This is true. Therefore, thinking of the changes as a safety measure and way to detect the source of the problem as [explained in my previous comment](#discussion_r626277254) could be the rationale for adding a separate method for invalidation.
--
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 #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#discussion_r625793222
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/EntryImpl.java
##########
@@ -166,7 +170,16 @@ protected void deallocate() {
timestamp = -1;
ledgerId = -1;
entryId = -1;
+ invalidated.set(false);
recyclerHandle.recycle(this);
}
+ @Override
+ public boolean invalidate() {
+ if (invalidated.compareAndSet(false, true)) {
+ release();
+ return true;
+ }
+ return false;
Review comment:
I don't think this is the correct approach. If the issue is that we're using an already released buffer, we should fix that instead.
This will avoid decrementing the ref-count more than once, but it will not prevent the 2nd thread from accessing an entry whose ref-count was already to 0.
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/RangeCache.java
##########
@@ -113,7 +113,7 @@ public Value get(Key key) {
try {
value.retain();
values.add(value);
- } catch (Throwable t) {
+ } catch (IllegalReferenceCountException e) {
Review comment:
It's expected to happen and it's fine when it happens. It just indicates the entry is being evicted when we're trying to access it. If the retain succeeds, the operation was successful, otherwise the entry is already gone.
--
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 edited a comment on pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
merlimat edited a comment on pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#issuecomment-833757513
> As mentioned earlier, that is prevented with AbstractCASReferenceCounted in Pulsar in most cases.
At this point, we can actually get rid of `AbstractCASReferenceCounted`. It was added in #2995 as a temporary measure to work around a change in behavior in Netty. The Netty issue was then fixed in 4.1.32 and we don't need the special treatment anymore.
The change https://github.com/apache/pulsar/blob/dcaa1d350fec920ef0ba32b4aeecd2e9604d4824/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/EntryCacheImpl.java#L187-L196 was added as a way to handle the same bug, but I agree that it's really dangerous in that we don't really what got wrong, as in where the corruption was...
As for the 2nd part (the Cursor.readEntryFailed), that seems ok to me. The `entries` there are the partial entries that were already read before. They might be coming from the cache or directly from bookies. In any case, the ref-count was increased when they came out of the cache, so we're required to release (and it should be safe to do so).
--
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 pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#issuecomment-833439133
It seems that here's a location in code where a "double release" can occur:
https://github.com/apache/pulsar/blob/d2138f7c4a84bc4d8b1d07600af15b30b7e2416e/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L1205-L1242
more specifically on line 1214:
https://github.com/apache/pulsar/blob/d2138f7c4a84bc4d8b1d07600af15b30b7e2416e/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L1214
and lines 1234-1237:
https://github.com/apache/pulsar/blob/d2138f7c4a84bc4d8b1d07600af15b30b7e2416e/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L1234-L1237
--
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 a change in pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#discussion_r626277254
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/EntryImpl.java
##########
@@ -166,7 +170,16 @@ protected void deallocate() {
timestamp = -1;
ledgerId = -1;
entryId = -1;
+ invalidated.set(false);
recyclerHandle.recycle(this);
}
+ @Override
+ public boolean invalidate() {
+ if (invalidated.compareAndSet(false, true)) {
+ release();
+ return true;
+ }
+ return false;
Review comment:
This is a safety measure to prevent races in invalidating the entries. I agree that the issues in releasing must be fixed. The benefit of adding a separate method for invalidation would help detect when the problem is caused by invalidating the entry twice. Some logging could be added to detect the issues where there's a race in invalidation which causes a "double release".
Currently, it seems that the problems that we are seeing could occur only when there's a race in invalidation. At a quick glance, there doesn't seem to be other code paths where the entry is released but not retained as part of the same "flow".
Would this justify adding some extra protection against races in invalidation?
--
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 pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#issuecomment-834093102
> At this point, we can actually get rid of AbstractCASReferenceCounted. It was added in #2995 as a temporary measure to work around a change in behavior in Netty. The Netty issue was then fixed in 4.1.32 and we don't need the special treatment anymore.
I'm thinking of replacing it with something that would give extra protection against bugs. Let's see what it evolves into. I'll push changes to this PR once there's something presentable.
> was added as a way to handle the same bug, but I agree that it's really dangerous in that we don't really what got wrong, as in where the corruption was...
>
> As for the 2nd part (the Cursor.readEntryFailed), that seems ok to me. The entries there are the partial entries that were already
About these 2 lines of code together:
```
invalidateAllEntries(lh.getId());
callback.readEntryFailed(createManagedLedgerException(t), ctx);
```
The problem here seems to be that invalidateAllEntries will call `.release()` and then the `callback.readEntryFailed` will also call `.release()` for the same set of entries and this leads to the "double release" which can cause the entry to be returned to the recycler (when there's one more outstanding usage of the entry). Once it's returned to the recycler, it can get used in some other usage at the same time. After that, the outstanding usage of the entry in the first place calls `.release()` and it could lead to the NPE that was reported. Makes sense?
--
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 pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
lhotari removed a comment on pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#issuecomment-833439133
It seems that here's a location in code where a "double release" can occur:
https://github.com/apache/pulsar/blob/d2138f7c4a84bc4d8b1d07600af15b30b7e2416e/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L1205-L1242
more specifically on line 1214:
https://github.com/apache/pulsar/blob/d2138f7c4a84bc4d8b1d07600af15b30b7e2416e/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L1214
and lines 1234-1237:
https://github.com/apache/pulsar/blob/d2138f7c4a84bc4d8b1d07600af15b30b7e2416e/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L1234-L1237
--
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 pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#issuecomment-834104386
> The problem here seems to be that invalidateAllEntries will call .release() and then the callback.readEntryFailed will also call .release() for the same set of entries and this leads to the "double release"
I don't think there's a double release because:
1. `invalidateAllEntries` is invalidating, thus removing and releasing the entries kept in cache
2. `callback.readEntryFailed` is releasing entries that were already out of the cache. We're just release the additional ref-count that was added when the entry came out of the cache (since we're not going to use the entry).
The main traits of the entry cache are:
1. The cache has ownership of the entry and keeps 1 ref-count when the entry is cached.
2. To grab an entry from the cache, we try to `retain()` (which might throw if the entry is being deallocated)
3. To invalidate, entries have to first get removed from the map and then release 1 ref-count
--
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] dlg99 commented on pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
dlg99 commented on pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#issuecomment-832119059
@lhotari I agree with @merlimat on " If the issue is that we're using an already released buffer, we should fix that instead."
It could be caused by i.e. an implementation of the ReadEntriesCallback/ReadEntryCallback/whatever other callback that is expected to properly release entry. Or, as you mentioned at the issue, it could be https://github.com/netty/netty/issues/10986
Other issue is: do we have a repro?
Have we confirmed that upgrade of Netty to a version with fix https://github.com/netty/netty/issues/10986 doesn't help?
and that this fix helps?
--
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 a change in pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#discussion_r625751106
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/RangeCache.java
##########
@@ -90,7 +90,7 @@ public Value get(Key key) {
try {
value.retain();
return value;
- } catch (Throwable t) {
+ } catch (IllegalReferenceCountException e) {
Review comment:
probably we should log something here.
this case must not happen
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/RangeCache.java
##########
@@ -113,7 +113,7 @@ public Value get(Key key) {
try {
value.retain();
values.add(value);
- } catch (Throwable t) {
+ } catch (IllegalReferenceCountException e) {
Review comment:
probably we should log something here.
this case must not happen
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/RangeCache.java
##########
@@ -168,9 +170,11 @@ public Value get(Key key) {
}
Value value = entry.getValue();
- ++removedEntries;
- removedSize += weighter.getSize(value);
- value.release();
+ long entrySize = weighter.getSize(value);
+ if (value.invalidate()) {
Review comment:
I am afraid we are only hiding the problem.
there must be some coordination about these entries, some clear protocol about who is the owner of the entry.
when we get to this point then we must be sure that the refcount is valid on the value, otherwise it is always an hazard.
I believe that the right protocol is that `before` calling` weighter.getSize(value);` we should `try` to acquire the entry and in case of failure we can ignore the entry.
```
Value value = entry.getValue();
if (value.tryAcquire()) {
++removedEntries;
removedSize += weighter.getSize(value);
value.release(); // this refers to the value.tryAcquire()
}
```
when we remove the entry we must have some write lock over the entry, that prevents double releases
--
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 pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#issuecomment-832224495
Actually, I'm not 100% sure that the having invalidations called by multiple thread could lead to the issue. In all the cases the entries are removed from the `ConcurrentSkipList` before getting release, and the guarantee there is that the removal should be atomic.
--
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 pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#issuecomment-833757513
> As mentioned earlier, that is prevented with AbstractCASReferenceCounted in Pulsar in most cases.
At this point, we can actually get rid of `AbstractCASReferenceCounted`. It was added in #2995 as a temporary measure to work around a change in behavior in Netty. The Netty issue was then fixed in 4.1.32 and we don't need the special treatment anymore.
The change https://github.com/apache/pulsar/blob/dcaa1d350fec920ef0ba32b4aeecd2e9604d4824/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/EntryCacheImpl.java#L187-L196 was added as a way to handle the same bug, but I agree that it's really dangerous in that we don't really what got wrong.
As for the 2nd part (the Cursor.readEntryFailed), that seems ok to me. The `entries` there are the partial entries that were already read before. They might be coming from the cache or directly from bookies. In any case, the ref-count was increased when they came out of the cache, so we're required to release (and it should be safe to do so).
--
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