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

Fix for issue #117 #114 #113 #88 #120

Merged
merged 11 commits into from
May 19, 2014
Merged

Conversation

NiteshKant
Copy link
Member

Issue #117: This was a major issue with the RxClient implementation which was mutating (updating ChannelInitializer) in the bootstrap for every connect. The reason for doing this was to have a ClientLifecycleListener provide connection events. This design was naively taken from the RxServer but does not apply for the client as the LifecycleListener requires an observer which changes for the client per connection.
This change fixes this issue by removing the newConnection notification responsibility from the LifecycleListener and have it done by the ClientChannelFactory. This also enabled to remove a few other redundant constructs like the AbstractChannelFactory which was unnecessarily complex. So, the code looks much cleaner now :)

Issue #114: rx-netty core was using numerus for LongAdder implementation. Now it uses netty's internal backport of LongAdder.

Issue #113: Rewrote the Redirect implementation as an Rx operator. This makes the code much cleaner and provides a very useful feature of being able to elect which HttpClient implementation must be used for executing redirects. This was a major limitation of the previous approach.

Issue #88: Provide an implementation of CompositeHttpClient which is a composite of HttpClients per ServerInfo. This provides additional submit() methods which also takes a host, port. By default this client submits requests to localhost.
RxNetty now has two flavors of createHttpRequest() that takes a HttpClientRequest with the full URI set and uses an appropriate HttpClient instance to execute this request.
Now, one can use a shorthand:

RxNetty.createHttpRequest(HttpClientRequest.createGet("http://localhost:8080/hello"))

to submit a request with full URI which was not previously possible.

@cloudbees-pull-request-builder

RxNetty-pull-requests #39 FAILURE
Looks like there's a problem with this pull request

@Override
public boolean requiresRedirect(RedirectionContext context, HttpClientResponse<O> response) {
int statusCode = response.getStatus().code();
return statusCode >= 301 && statusCode <= 308;

Choose a reason for hiding this comment

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

304 is for HTTP caching, not for redirect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I updated the code to only redirect for 301,302,303, 307, 308.

Nitesh Kant added 2 commits May 19, 2014 09:47
@cloudbees-pull-request-builder

RxNetty-pull-requests #42 FAILURE
Looks like there's a problem with this pull request

@benjchristensen
Copy link
Member

How about for the most common cases we have overloads like this?

RxNetty.createHttpGet("http://localhost:8080/hello")

@NiteshKant
Copy link
Member Author

@benjchristensen yes I will add that method. The caveat being that if the user wants to add any header, the HttpClientRequest builder has to be used.
I will also add similar method for DELETE.
Shorthands for PUT & POST becomes a bit more complex as we have a few ways of specifying the content. So, I will stay away from them as of now.

I personally feel we should give a bit more thought on making this more usable by providing a more fluent DSL more on the lines you suggested offline i.e. by having an extension to Observable. I will put together my thoughts on that and open a different issue.

@cloudbees-pull-request-builder

RxNetty-pull-requests #43 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

RxNetty-pull-requests #45 SUCCESS
This pull request looks good

NiteshKant added a commit that referenced this pull request May 19, 2014
@NiteshKant NiteshKant merged commit 42eafc4 into ReactiveX:master May 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants