aws / aws/aws-sdk-java-v2

Netty Async Client marker to avoid ByteBuf copy

Open
#2,052 2 comments 0 reactions 0 assignees View on GitHub
feature-request needs-discussion p3
Dominant language
Java
Stars
2.6k
Forks
1k
Avg merge
2d 9h
Merged PRs (30d)
51

Description

The ResponsePublisher.PublisherAdapter conservatively copy the content of the ByteBuf to a new ByteBuffer to pass to
the subscriber in case the consumption will be async.

## Describe the Feature
Use a marker interface or annotation or a configuration option to indicate that subscriber is not async and we can then use the ByteBuf.nioBuffer instead of doing a copy of the buffer. That will reduce allocation and the work needed to download file for non async subscriber.

## Is your Feature Request related to a problem?

## Proposed Solution
There does not seem to be a way to indicate if the subscription is async or not in the reactive stream api - but I'm not very well verse with that.

```
@Override
public void subscribe(Subscriber subscriber) {
boolean isAsync = isAsyncSubscriber(subscriber);

```

so with an marker interface that could look like that
```
// assume async unless mark otherwise
public isAsyncSubscriber(Subscriber subscriber) {
return subscriber instanceof SyncSubscriber;

}
```

the on next method can check an isAsync flag
```
@Override
public void onNext(HttpContent httpContent) {
// isDone may be true if the subscriber cancelled
if (isDone.get()) {
httpContent.release();
return;
}

// Needed to prevent use-after-free bug if the subscriber's onNext is asynchronous
ByteBuffer byteBuffer =
tryCatchFinally(() -> isAsync? copyToByteBuffer(httpContent.content()) : httpContent.content().nioBuffer() ,
this::onError,
httpContent::release);

//As per reactive-streams rule 2.13, we should not call subscriber#onError when
//exception is thrown from subscriber#onNext
if (byteBuffer != null) {
tryCatch(() -> subscriber.onNext(byteBuffer),
this::notifyError);
}
}
```

## Describe alternatives you've considered

- could use annotation, but the reflection cost would be quite high
- could use as part of the configuration, but that would then apply globally
- could use as part of the GetObjectRequest but that seems pretty far from the code it concern

## Additional Context

The goal is to reduce the load, Netty has its own allocator, but we are losing the benefits of that by forcing a copy for the async case.

- [X] I may be able to implement this feature request

## Your Environment

* AWS Java SDK version used:
* JDK version used:
* Operating System and version:

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.