GEODE-5565: Release off heap memory if unable to set value #2305
GEODE-5565: Release off heap memory if unable to set value #2305jhuynh1 merged 2 commits intoapache:developfrom
Conversation
| value = this.prepareValueForCache(region, newValueToWrite, false); | ||
| setValue(region, value); | ||
| } catch (RegionDestroyedException e) { | ||
| if (value != null && value instanceof StoredObject) { |
There was a problem hiding this comment.
I think the null check is not needed because null is never an "instanceof" anything.
There was a problem hiding this comment.
It is actually the job of the "setValue" method to release the offheap value it is given if it can not store it in the entry.
I see it calls "releaseOffHeapRefIfRegionBeingClosedOrDestroyed" so why didn't that work?
Did the RegionDestroyedException come from somewhere else?
Doing it the way you have it could lead to extra releases. I suggest you figure out why setValue did not release it since it was responsible for doing so and not to change the code at this location.
There was a problem hiding this comment.
The reguo destroyed exception is being thrown in _setValue. This is called before the releaseoffheap... Method is called. I can add the try catch to abstractregionentry instead. The problem was the release here was not getting a chance to execute. The stack trace is in the ticket.
Ill move the try catch lower and see if that resolves the issue
There was a problem hiding this comment.
I don't think it is _setValue.
I think the bug is in the disk implementation of setValue:
at org.apache.geode.internal.cache.entries.DiskEntry$Helper.update(DiskEntry.java:856)
and also in basicUpdate that this method calls.
If either of these throw an exception before storing the new value in the entry then they are responsible for releasing the new value if it is a StoredObject.
| setValue(region, this.prepareValueForCache(region, newValueToWrite, false)); | ||
| Object value = null; | ||
| try { | ||
| value = this.prepareValueForCache(region, newValueToWrite, false); |
There was a problem hiding this comment.
Why not call prepareValueForCache on the line that declares value?
Object value = prepareValueForCache...
No need to catch anything from prepareValueForCache because in that case it did not return anything.
There was a problem hiding this comment.
Also you can remove "this." on the method call
jhuynh1
force-pushed
the
feature/GEODE-5565
branch
from
73cb9d4 to
c8cc2d5
Compare
jhuynh1
force-pushed
the
feature/GEODE-5565
branch
2 times, most recently
from
762d282 to
ec9fe5f
Compare
2 participants
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun cleanly?Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.