[#1472][part-3] fix(client): Fix occasional IllegalReferenceCountException issues in extremely rare scenarios

[rickyma]

What changes were proposed in this pull request?

Improve the robustness of methods ShuffleDataResult.release() and ShuffleIndexResult.release() to fix occasional IllegalReferenceCountException issues in extremely rare scenarios.

Why are the changes needed?

A sub PR for: #1519

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing UTs.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (576a925) 54.27% compared to head (5cfa1c8) 55.18%.
Report is 11 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1522      +/-   ##
============================================
+ Coverage     54.27%   55.18%   +0.91%     
+ Complexity     2807     2806       -1     
============================================
  Files           427      410      -17     
  Lines         24349    22057    -2292     
  Branches       2077     2081       +4     
============================================
- Hits          13215    12172    -1043     
+ Misses        10305     9125    -1180     
+ Partials        829      760      -69     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[EnricoMi]

What if we would make sure that this.buffer.release() is called at most once?

  public synchronized void release() {
    if (this.buffer != null) {
      this.buffer.release();
      this.buffer = null;
    }
  }

Then we wouldn't need to add logic that requires internal knowledge of this.buffer.release().

[github-actions]

Test Results

2 287 files  ±0  2 287 suites  ±0   4h 30m 0s ⏱️ +17s
  819 tests ±0    818 ✅ ±0   1 💤 ±0  0 ❌ ±0 
9 086 runs  ±0  9 073 ✅ ±0  13 💤 ±0  0 ❌ ±0 

Results for commit 5cfa1c8. ± Comparison against base commit 3954ce7.

♻️ This comment has been updated with latest results.

[rickyma]

What if we would make sure that this.buffer.release() is called at most once?

  private boolean released = false;

  public synchronized void release() {
    if (! released && this.buffer != null) {
      this.buffer.release();
      released = true;
    }
  }

Then we wouldn't need to add logic that requires internal knowledge of this.buffer.release().

Your approach might not solve this issue. Because my previous code was written in a similar way to yours.
Currently, methods ShuffleDataResult.release() and ShuffleIndexResult.release() are called only once(at least, that's what the code suggests.), but IllegalReferenceCountException will still arise in extremely rare scenarios occasionally(not always). That means the refCnt of ByteBuf inside ShuffleDataResult and ShuffleIndexResult is already 0 when the first time methods ShuffleDataResult.release() and ShuffleIndexResult.release() are called.

The cause is unclear at the moment, we can investigate further in the future. But at least for now, the changes I've made will definitely not cause any problems and will solve this issue.

[rickyma]

We need this fix currently. It will not cause any side effects. @jerqi

[EnricoMi]

TBH, this looks more like a hack than a fix. Usually such issues won't be investigated further once a workaround is merged.

[rickyma]

TBH, this looks more like a hack than a fix. Usually such issues won't be investigated further once a workaround is merged.

I can add TODO in comments for now. It is a tricky bug. We need to fix this immediately.

[jerqi]

Could we add some logs to help us to find the root cause of this issue?

[rickyma]

The bug is fixed.

It is more like a follow-up PR for #1455.

The previous approach could not distinguish between an EMPTY_BUFFER and a ByteBuf with a zero value of readableBytes.

@jerqi PTAL

[jerqi]

LGTM.

[jerqi]

Merged to master. Thanks @rickyma for contribution! Thanks @EnricoMi for review!