[HBASE-26688] Threads shared EMPTY_RESULT may lead to unexpected client job down.

• Type: Bug
• Status: Resolved
• Priority: Major
• Resolution: Fixed
• Fix Version/s: 2.5.0, 3.0.0-alpha-3, 2.4.10
• Component/s: Client
• Hadoop Flags: Incompatible change, Reviewed

Release Note

Result#advance with empty cell list will always return false but not raise NoSuchElementException when called multiple times.
This is a behavior change so it is an 'incompatible change', but since it will not introduce any compile error and the old behavior is 'broken', so we also fix it for current release branches.

• Assignee: Yutong Xiao
• Reporter: Yutong Xiao
• Created: 20/Jan/22 03:40
• Updated: 12/Mar/22 19:30
• Resolved: 15/Feb/22 12:40

Description

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:

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);

Result#advance

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");
  }

Result#current

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];
  }

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.

Issue Links

• is blocked by: HBASE-26709 Ban the usage of junit 3 TestCase
• links to: GitHub Pull Request #4048
• links to: GitHub Pull Request #4072
• links to: GitHub Pull Request #4073