You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "rickyma (via GitHub)" <gi...@apache.org> on 2024/02/12 07:37:07 UTC
[PR] [#1472][part-3] fix(client): Fix occasional IllegalReferenceCountException issues in extremely rare scenarios [incubator-uniffle]
rickyma opened a new pull request, #1522:
URL: https://github.com/apache/incubator-uniffle/pull/1522
### 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: https://github.com/apache/incubator-uniffle/pull/1519
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing UTs.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1472][part-3] fix(client): Fix occasional IllegalReferenceCountException issues in extremely rare scenarios [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1522:
URL: https://github.com/apache/incubator-uniffle/pull/1522#issuecomment-1940751695
> 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.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1472][part-3] fix(client): Fix occasional IllegalReferenceCountException issues in extremely rare scenarios [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1522:
URL: https://github.com/apache/incubator-uniffle/pull/1522#issuecomment-1943190721
The bug is fixed.
It is more like a follow-up PR for [#1455](https://github.com/apache/incubator-uniffle/pull/1455).
The previous approach could not distinguish between an `EMPTY_BUFFER` and a `Buffer` with a `readableBytes` value of zero.
@jerqi 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.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1472][part-3] fix(client): Fix occasional IllegalReferenceCountException issues in extremely rare scenarios [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi merged PR #1522:
URL: https://github.com/apache/incubator-uniffle/pull/1522
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1472][part-3] fix(client): Fix occasional IllegalReferenceCountException issues in extremely rare scenarios [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1522:
URL: https://github.com/apache/incubator-uniffle/pull/1522#issuecomment-1940758744
> 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.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1472][part-3] fix(client): Fix occasional IllegalReferenceCountException issues in extremely rare scenarios [incubator-uniffle]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on PR #1522:
URL: https://github.com/apache/incubator-uniffle/pull/1522#issuecomment-1938197892
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()`.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1472][part-3] fix(client): Fix occasional IllegalReferenceCountException issues in extremely rare scenarios [incubator-uniffle]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1522:
URL: https://github.com/apache/incubator-uniffle/pull/1522#issuecomment-1938212918
## Test Results
2 287 files ±0 2 287 suites ±0 4h 27m 34s :stopwatch: -11s
819 tests ±0 818 :white_check_mark: ±0 1 :zzz: ±0 0 :x: ±0
9 086 runs ±0 9 073 :white_check_mark: ±0 13 :zzz: ±0 0 :x: ±0
Results for commit 224f79bd. ± Comparison against base commit 866f5ff1.
[test-results]:data:application/gzip;base64,H4sIANDTyWUC/03MTQ6DIBCG4asY1l0MowjTyzQikJD60yCsTO9eNEhdfg/DuzPnJ7uxZ4Oo5KNhW/Lxvk0KQ/TrkoX3ILpM+T0eF4rTtV5bGseT1J/e/nP8quAGP2WACjaENRQJaTmaBKov62oSyLZSaVa4Nc99T47rPPuYB0PsnCRtjGqRpDJcCxSkURopgHcaQDhOZNn3B+jZcM8QAQAA
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1472][part-3] fix(client): Fix occasional IllegalReferenceCountException issues in extremely rare scenarios [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1522:
URL: https://github.com/apache/incubator-uniffle/pull/1522#issuecomment-1940434909
We need this fix currently. It will not cause any side effects. @jerqi
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1472][part-3] fix(client): Fix occasional IllegalReferenceCountException issues in extremely rare scenarios [incubator-uniffle]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on PR #1522:
URL: https://github.com/apache/incubator-uniffle/pull/1522#issuecomment-1940497967
TBH, this looks more like a hack than a fix. Usually such issues won't be investigated further once a workaround is merged.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1472][part-3] fix(client): Fix occasional IllegalReferenceCountException issues in extremely rare scenarios [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1522:
URL: https://github.com/apache/incubator-uniffle/pull/1522#discussion_r1487874179
##########
common/src/main/java/org/apache/uniffle/common/ShuffleDataResult.java:
##########
@@ -106,7 +106,7 @@ public boolean isEmpty() {
}
public void release() {
- if (this.buffer != null) {
+ if (this.buffer != null && this.getDataBuf() != null && this.getDataBuf().refCnt() > 0) {
Review Comment:
Could we add some logs to help us to find the root cause of this 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.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1472][part-3] fix(client): Fix occasional IllegalReferenceCountException issues in extremely rare scenarios [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1522:
URL: https://github.com/apache/incubator-uniffle/pull/1522#issuecomment-1945325110
Merged to master. Thanks @rickyma for contribution! Thanks @EnricoMi for review!
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1472][part-3] fix(client): Fix occasional IllegalReferenceCountException issues in extremely rare scenarios [incubator-uniffle]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1522:
URL: https://github.com/apache/incubator-uniffle/pull/1522#issuecomment-1938183595
## [Codecov](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1522?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `4 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`576a925`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/576a925a21576467ab8713ca1d4b569d0a0af5c8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 54.27% compared to head [(`224f79b`)](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1522?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 54.72%.
> Report is 8 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1522?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../org/apache/uniffle/common/ShuffleIndexResult.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1522?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9TaHVmZmxlSW5kZXhSZXN1bHQuamF2YQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1522?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...a/org/apache/uniffle/common/ShuffleDataResult.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1522?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9TaHVmZmxlRGF0YVJlc3VsdC5qYXZh) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1522?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...pache/uniffle/server/ShuffleServerGrpcService.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1522?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlU2VydmVyR3JwY1NlcnZpY2UuamF2YQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1522?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #1522 +/- ##
============================================
+ Coverage 54.27% 54.72% +0.44%
+ Complexity 2807 2273 -534
============================================
Files 427 353 -74
Lines 24349 15813 -8536
Branches 2077 1451 -626
============================================
- Hits 13215 8653 -4562
+ Misses 10305 6665 -3640
+ Partials 829 495 -334
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1522?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1472][part-3] fix(client): Fix occasional IllegalReferenceCountException issues in extremely rare scenarios [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1522:
URL: https://github.com/apache/incubator-uniffle/pull/1522#issuecomment-1938215886
> 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.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org