Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BatcherImpl.add can hang if an exception is thrown starting a call #1324

Closed
igorbernstein2 opened this issue Oct 18, 2022 · 0 comments · Fixed by #1166
Closed

BatcherImpl.add can hang if an exception is thrown starting a call #1324

igorbernstein2 opened this issue Oct 18, 2022 · 0 comments · Fixed by #1166
Assignees
Labels
gax priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@igorbernstein2
Copy link
Contributor

Environment details

  • Programming language: java
  • OS: MacOS 12.6
  • Language runtime version: 11.0.16.1
  • Package version: 2.19.2

Steps to reproduce

Failing unit test:

BatchingDescriptor<Object, Object, Object, Object> batchingDescriptor = new BatchingDescriptor<Object, Object, Object, Object>() {
      @Override
      public BatchingRequestBuilder<Object, Object> newRequestBuilder(Object o) {
        return new BatchingRequestBuilder<Object, Object>() {
          @Override
          public void add(Object o) { }

          @Override
          public Object build() {
            return new Object();
          }
        };
      }

      @Override
      public void splitResponse(Object o, List<BatchEntry<Object, Object>> list) {
        for (BatchEntry<Object, Object> e : list) {
          e.getResultFuture().set(new Object());
        }
      }

      @Override
      public void splitException(Throwable throwable, List<BatchEntry<Object, Object>> list) {
        for (BatchEntry<Object, Object> e : list) {
          e.getResultFuture().setException(new RuntimeException("fake"));
        }
      }

      @Override
      public long countBytes(Object o) {
        return 1;
      }
    };

    UnaryCallable<Object, Object> unaryCallable =new UnaryCallable<Object, Object>() {
      @Override
      public ApiFuture<Object> futureCall(Object o, ApiCallContext apiCallContext) {
        throw new RuntimeException("this is not bubbled up!");
      }
    };
    Object prototype = new Object();
    BatchingSettings batchingSettings = BatchingSettings.newBuilder()
        .setDelayThreshold(Duration.ofSeconds(1))
        .setElementCountThreshold(100L)
        .setRequestByteThreshold(100L)
        .setFlowControlSettings(
            FlowControlSettings.getDefaultInstance()
        )
        .build();
    ScheduledExecutorService executor = Executors.newScheduledThreadPool(2);
    FlowController flowController = new FlowController(batchingSettings.getFlowControlSettings());
    ApiCallContext callContext = FakeCallContext.createDefault();

    BatcherImpl<Object, Object, Object, Object> batcher = new BatcherImpl<>(
        batchingDescriptor, unaryCallable, prototype, batchingSettings, executor, flowController, callContext
    );

    ApiFuture<Object> f = batcher.add(new Object());
    // get never returns
    Assert.assertThrows(ExecutionException.class, f::get);

Proposed solution

BatcherImpl#sendOutstanding() should catch exceptions from futureCall() and treat it as if the RPC failed

@igorbernstein2 igorbernstein2 added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Oct 18, 2022
@diegomarquezp diegomarquezp self-assigned this Oct 21, 2022
diegomarquezp referenced this issue in googleapis/gax-java Oct 21, 2022
fixes #1826. When the BatcherImpl's unary caller throws an exception,
this will bubble up to response.get()
@ddixit14 ddixit14 transferred this issue from googleapis/gax-java Feb 8, 2023
@ddixit14 ddixit14 added the gax label Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gax priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
3 participants