#YTDB-165

[logart]

Motivation:

While implementing #YTDB-165, I have a place where I do casArray.get(pageIndex) to check if the page is in the readCache already.

This operation fails for all new pages since they are not in the array yet.

Based on the logic of the method, I will then cache those pages and add them to the array.

Solution:

Add a flag (I know this is bad, but I feel like copying the method is even worse...) to the get method of CASObjectArray, so it will define either throw on out of bound (array behaviour) or return null (map behaviour).

An alternative solution would be to create an atomic operation. Something along the lines like getOrSet which will get the existing value or (if it is missing) set a computed one.

https://youtrack.jetbrains.com/issue/YTDB-495/Make-CASObjectArray-behave-like-a-map

[gemini-code-assist]

Summary of Changes

Hello @logart, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the CASObjectArray class by introducing a flexible way to handle out-of-bounds index access. Previously, accessing an index beyond the array's size would always result in an ArrayIndexOutOfBoundsException. The changes now allow consumers to choose between the traditional exception-throwing behavior or a new "map-like" behavior where null is returned for non-existent elements, which is particularly useful for caching mechanisms where checking for existence without exceptions is desired.

Highlights

• New getOrNull method: Added a new public method getOrNull(int index) to CASObjectArray that returns null if the specified index is out of bounds, providing a map-like behavior for checking element existence without exceptions.
• Refactored get method: The existing public get(int index) method in CASObjectArray was refactored to use an internal private get method with a throwOnOutOfBounds flag, ensuring it continues to throw an ArrayIndexOutOfBoundsException for out-of-bounds access, maintaining its original array-like behavior.
• New Unit Tests: Introduced new unit tests in CASObjectArrayTest to verify both the exception-throwing behavior of get() and the null-returning behavior of getOrNull() when accessing out-of-bounds indices.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a getOrNull method to CASObjectArray to provide map-like behavior for out-of-bounds access, returning null instead of throwing an exception. This is achieved by adding a private helper method controlled by a boolean flag. The changes are logical and include corresponding unit tests. However, my review identified a pre-existing critical race condition in the get method's logic, which this PR touches. I have provided a detailed explanation and a suggested fix for this concurrency bug. I have also recommended adding a concurrent test case to prevent regressions in this area.

[gemini-code-assist]

There is a critical race condition in the get method's logic, which is preserved from the original implementation. The initial check this.size.get() <= index can cause get to fail incorrectly if it races with a concurrent add operation.

The Race Condition:
The add method increments the size after the new value has been set in the underlying array. If a get(newIndex) call occurs in the window between the value being set and the size being incremented, it will read the old size and incorrectly throw an ArrayIndexOutOfBoundsException instead of waiting for and returning the new value.

Proposed Fix:
The logic should be inverted: first, attempt to read the value. If it's null, then check the size to distinguish between an index that is truly out of bounds and an index where a value is concurrently being written.

Since the full method body is not part of this diff, I cannot provide an auto-applicable suggestion. Here is the recommended implementation for the entire private T get(int index, boolean throwOnOutOfBounds) method, which resolves the race condition while preserving your intended new functionality:

private T get(int index, boolean throwOnOutOfBounds) {
  final var containerIndex = 31 - Integer.numberOfLeadingZeros(index + 1);
  final var containerSize = 1 << containerIndex;
  final var indexInsideContainer = index + 1 - containerSize;

  AtomicReferenceArray<T> container = containers.get(containerIndex);
  if (container == null) {
      if (this.size.get() <= index) {
          if (throwOnOutOfBounds) {
              throw new ArrayIndexOutOfBoundsException("Requested " + index + ", size is " + this.size.get());
          }
          return null;
      }
      do {
          Thread.yield();
          container = containers.get(containerIndex);
      } while (container == null);
  }

  T value = container.get(indexInsideContainer);
  if (value == null) {
      if (this.size.get() <= index) {
          if (throwOnOutOfBounds) {
              throw new ArrayIndexOutOfBoundsException("Requested " + index + ", size is " + this.size.get());
          }
          return null;
      }
      do {
          Thread.yield();
          value = container.get(indexInsideContainer);
      } while (value == null);
  }

  return value;
}

This same underlying issue also affects the compareAndSet method.

[gemini-code-assist]

This test correctly verifies the behavior of get() for an out-of-bounds index. However, as noted in the review of CASObjectArray.java, the get method has a race condition. It would be highly beneficial to add a concurrent test case to expose this bug and verify its fix. Such a test would typically involve one thread calling add() while another concurrently calls get() for the index being added, asserting that get() does not throw an exception but waits and returns the correct value.

[gemini-code-assist]

Code Review

This pull request introduces a getOrNull method to CASObjectArray to provide map-like behavior for out-of-bounds access, returning null instead of throwing an exception. This is achieved by refactoring the existing get method to use a private helper method. The changes are logical and well-implemented. New unit tests are added to cover the new functionality. I've made one suggestion in the test file to use a more specific exception class for better test precision. The idea of a future getOrSet method, as mentioned in the description, is also a good one to further enhance the class's capabilities.

[gemini-code-assist]

For better precision, it's recommended to assert for the specific exception type that is thrown by the code under test. The get method throws ArrayIndexOutOfBoundsException, so the test should check for that instead of its superclass IndexOutOfBoundsException.

Suggested change

	Assert.assertThrows(IndexOutOfBoundsException.class, () -> array.get(0));
	Assert.assertThrows(ArrayIndexOutOfBoundsException.class, () -> array.get(0));