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

Simple netty based HttpClient #27881

Closed

Conversation

kasobol-msft
Copy link
Contributor

This PR has a prototype of a lightweight HttpClient based on Netty .
This prototype aims to build an http client that works for various types of payloads and context (sync vs async) and mostly serve sunny day scenarios while introducing all the required complexities to do so.
There's a lot of corner (mostly error) cases that remain uncovered here, but they likely won't affect existing complexity much.

Implemented features:

  • Reactor-less sync execution. Sync-over-async on the CompletableFeature instead.
  • Connection Pooling
  • Buffered requests handling
  • Stream-able requests handling
  • On demand response buffering
  • Piped based response streaming for sync calls
  • Sink based response streaming for async calls (there's still plenty room for improvement here around better backpressure handling).

The client seems to be functional, i.e. SimpleNettyHttpClientHttpClientTests extends HttpClientTests and SimpleNettyHttpClientRestProxyTests extends RestProxyTests are passing.

@ghost ghost added the Azure.Core azure-core label Mar 25, 2022
@azure-sdk
Copy link
Collaborator

API changes have been detected in com.azure:azure-core. You can review API changes here

@azure-sdk
Copy link
Collaborator

API changes have been detected in com.azure:azure-core-http-netty. You can review API changes here

API changes

+     public class SimpleNettyHttpClient implements HttpClient {
+         public SimpleNettyHttpClient() 
+         @Override public Mono<HttpResponse> send(HttpRequest request) 
+         @Override public Mono<HttpResponse> send(HttpRequest request, Context context) 
+         @Override public HttpResponse sendSynchronously(HttpRequest request, Context context) 
+     }

@azure-sdk
Copy link
Collaborator

API changes have been detected in com.azure:azure-core-test. You can review API changes here

outputStream = new ByteArrayOutputStream();
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A recommendation in "Effective Java (EJ)" while following DCL is to introduce a local variable. As per the EJ, such a version may improve the perf by about 25% compared to the version without the local variable.

The code would be (pattern copied from EJ)

public void collect(ByteBuf buffer, boolean isLast) {
    if (buffer.isReadable()) {
        ByteArrayOutputStream result = ensureStream();
        try {
            buffer.readBytes(result, buffer.readableBytes());
        } catch (IOException e) {
            throw new UncheckedIOException(e);
        }
    }
}

private ByteArrayOutputStream ensureStream() {
    ByteArrayOutputStream result = outputStream;
    if (result == null) {
        synchronized (this) {
            result = outputStream;
            if (result == null) {
                outputStream = result = new ByteArrayOutputStream();
            }
        }
    }
    return result;
}

@azure-sdk azure-sdk deleted the branch Azure:feature/sync-stack May 18, 2022 16:13
@azure-sdk azure-sdk closed this May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core azure-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants