[
https://issues.apache.org/jira/browse/HBASE-26688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17483509#comment-17483509
]
Yutong Xiao commented on HBASE-26688:
-------------------------------------
For non-empty Result objects, the logic is the same after the change.
This only changes the behaviour of Result with empty cell list, which used to
have a thread safety bug.
A more common usage is to check the return value of Result#advance once first,
if it is false, the client should search the next result.
This usage is impacted by this bug. Even if the client wants to catch the
exception, they may get the exception at the first time of advance() but not
the second time, which is not the original logic.
By the way, in the original code, if the cell list is null, the method also
always returns false and never raises the exception.
Result#isEmpty with null cell list and Result#isEmpty with empty list all
return true. But in the original code, "empty" Results have different
behaviours.
This is actually a bug.
If we want to keep return NoSuchElementException for Result with empty list. My
opinion is we can change the Result class thread safe. (e.g. make
Result#cellScannerIndex threadlocal?)
> Threads shared EMPTY_RESULT may lead to unexpected client job down.
> -------------------------------------------------------------------
>
> Key: HBASE-26688
> URL: https://issues.apache.org/jira/browse/HBASE-26688
> Project: HBase
> Issue Type: Bug
> Components: Client
> Reporter: Yutong Xiao
> Assignee: Yutong Xiao
> Priority: Major
> Fix For: 2.5.0, 3.0.0-alpha-3, 2.4.10
>
>
> Currently, we use a pre-created EMPTY_RESULT in the ProtoBuf.util to reduce
> the object creation. But these objects could be shared by multi client
> threads. The Result#cellScannerIndex related methods could throw confusing
> exception and make the client job down. Could refine the logic of these
> methods.
> The precreated objects in ProtoBufUtil.java:
> {code:java}
> private final static Cell[] EMPTY_CELL_ARRAY = new Cell[]{};
> private final static Result EMPTY_RESULT = Result.create(EMPTY_CELL_ARRAY);
> final static Result EMPTY_RESULT_EXISTS_TRUE = Result.create(null, true);
> final static Result EMPTY_RESULT_EXISTS_FALSE = Result.create(null, false);
> private final static Result EMPTY_RESULT_STALE =
> Result.create(EMPTY_CELL_ARRAY, null, true);
> {code}
> Result#advance
> {code:java}
> public boolean advance() {
> if (cells == null) return false;
> cellScannerIndex++;
> if (cellScannerIndex < this.cells.length) {
> return true;
> } else if (cellScannerIndex == this.cells.length) {
> return false;
> }
> // The index of EMPTY_RESULT could be incremented by multi threads and
> throw this exception.
> throw new NoSuchElementException("Cannot advance beyond the last cell");
> }
> {code}
> Result#current
> {code:java}
> public Cell current() {
> if (cells == null
> || cellScannerIndex == INITIAL_CELLSCANNER_INDEX
> || cellScannerIndex >= cells.length)
> return null;
> // Although it is almost impossible,
> // We can arrive here when the client threads share the common reused
> EMPTY_RESULT.
> return this.cells[cellScannerIndex];
> }
> {code}
> In this case, the client can easily got confusing exceptions even if they use
> different connections, tables in different threads.
> We should change the if condition cells == null to isEmpty() to avoid the
> client crashed from these exception.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
