Added asynchronous (NIO) based authentication support. #283

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
3 participants
@brettwooldridge

Hi Fernandez,

First thanks for the excellent library. The code was very easy to understand and work with. I believe my changes adhere to your style correctly.

Let me summarize what I've done. Basically, to increase scalability I added support for asynchronous authentication using the Apache HTTP Components NIO library. This allows a single thread on the server to have hundreds or thousands of in-flight authentications.

You'll notice in the pom.xml there is a new entry:

<dependency>
  <groupId>org.apache.httpcomponents</groupId>
  <artifactId>httpcore-nio</artifactId>
  <version>4.2.1</version>
  <scope>compile</scope>
</dependency>

This is a compile-time dependency, but not a run-time dependency. The library can be used as is, without asynchronous support. However, if the Apache HTTP Components (httpcore-nio) library is available, the user has the option of using a new Async flavor of the API. The original API is unchanged, and uses the existing code (HttpURLConnection), even in the presence of the Apache components. It is only if the developer explicitly uses the Async flavor of the API that they will get asynchronous behavior.

I have added a TwitterAsyncExample and FacebookAsyncExample where you can see what it looks like from the end-user perspective. I think the usage is fairly crisp.

Now to the code. I know it looks like I changed a lot of files, but 90% was just moving a few things around to allow for code-sharing. Basically this is the broad strokes:

Request was changed to an interface. The majority of the code (+80%) in the old Request class was pulled into RequestBase, with the remainder moved to RequestSync. Essentially, wrt the original Request that is all that was done. RequestSync and RequestAsync extend RequestBase. By changing Request to an interface, with the same signature as the original class, it was possible to add the two implementation classes without affecting any client code.

OAuthRequest has a new constructor:

public OAuthRequest(Verb verb, String url, boolean async)

which allows creating an asynchronous flavor of an OAuthRequest. OAuthRequest now holds a Request (interface) reference as a member and delegates to it. The Request can obviously be either a RequestSync or RequestAsync implementation. There is some minor reflection to ensure there is no hard dependency on the RequestAsync class (which would pull in Apache requirements).

The ServiceBuilder has a new method:

public OAuthServiceAsync buildAsync();

which returns a asynchronous flavor of the OAuthService.

My suggestion is, before looking at the diffs, just make a local clone of my repository and take a look at the code. I find looking at diffs where classes where split (such as Request into RequestBase and RequestSync) confusing.

I welcome your feedback and suggestions.

@cndoublehero

This comment has been minimized.

Show comment Hide comment
@cndoublehero

cndoublehero Jul 6, 2012

cool.

cool.

brettwooldridge added some commits Jul 7, 2012

Merge branch 'master' of https://github.com/brettwooldridge/scribe-java
Conflicts:
	src/test/java/org/scribe/examples/FacebookAsyncExample.java
	src/test/java/org/scribe/examples/TwitterAsyncExample.java
@fernandezpablo85

This comment has been minimized.

Show comment Hide comment
@fernandezpablo85

fernandezpablo85 Jul 10, 2012

Collaborator

Hey man, this is awesome.

Right now I have some work in progress to separate the Request interface from its implementations, this was originally thought to support commons http (not async), but it shouldn't be hard to make the solution work for both.

I can integrate this once the changes are in place.

Collaborator

fernandezpablo85 commented Jul 10, 2012

Hey man, this is awesome.

Right now I have some work in progress to separate the Request interface from its implementations, this was originally thought to support commons http (not async), but it shouldn't be hard to make the solution work for both.

I can integrate this once the changes are in place.

@brettwooldridge

This comment has been minimized.

Show comment Hide comment
@brettwooldridge

brettwooldridge Aug 22, 2012

Any work done so far on this pull request? Just checking in since the last comment was a month ago.

Any work done so far on this pull request? Just checking in since the last comment was a month ago.

@fernandezpablo85

This comment has been minimized.

Show comment Hide comment
@fernandezpablo85

fernandezpablo85 Aug 22, 2012

Collaborator

Yes, sorry. I'm still looking for an elegant way to support multiple Http implementations (nio, commons-http, etc). I plan to do this for Scribe 2.0.

This pull request is super useful don't get me wrong, but the approach makes a class explosion that doesn't scale for multiple request implementations. Sorry

Collaborator

fernandezpablo85 commented Aug 22, 2012

Yes, sorry. I'm still looking for an elegant way to support multiple Http implementations (nio, commons-http, etc). I plan to do this for Scribe 2.0.

This pull request is super useful don't get me wrong, but the approach makes a class explosion that doesn't scale for multiple request implementations. Sorry

@brettwooldridge

This comment has been minimized.

Show comment Hide comment
@brettwooldridge

brettwooldridge Aug 22, 2012

If you have suggestions, I'm willing to put in some refactoring time.

In defence of the "class explosion", this pull request only introduces 5 classes: RequestBase, RequestSync, RequestAsync, OAuthServiceAsync, and ResponseCallback. I think regardless of implementation you're going to wind up with something like OAuthServiceAsync, and true asynchronicity demands a callback mechanism like ResponseCallback.

RequestBase was introduced because of the amount of common code between synchronous and asynchronous requests ... with the resulting RequestSync class containing only 117 lines total.

If you have suggestions, I'm willing to put in some refactoring time.

In defence of the "class explosion", this pull request only introduces 5 classes: RequestBase, RequestSync, RequestAsync, OAuthServiceAsync, and ResponseCallback. I think regardless of implementation you're going to wind up with something like OAuthServiceAsync, and true asynchronicity demands a callback mechanism like ResponseCallback.

RequestBase was introduced because of the amount of common code between synchronous and asynchronous requests ... with the resulting RequestSync class containing only 117 lines total.

@fernandezpablo85

This comment has been minimized.

Show comment Hide comment
@fernandezpablo85

fernandezpablo85 Aug 22, 2012

Collaborator

Don't get me wrong. I'm not against code quality here, looks like really nice and carefully crafted code indeed. It's the approach that doesn't convince me but not because of your patch but because scribe was never intended to have more than one implementation of an http client.

Collaborator

fernandezpablo85 commented Aug 22, 2012

Don't get me wrong. I'm not against code quality here, looks like really nice and carefully crafted code indeed. It's the approach that doesn't convince me but not because of your patch but because scribe was never intended to have more than one implementation of an http client.

@brettwooldridge

This comment has been minimized.

Show comment Hide comment
@brettwooldridge

brettwooldridge Aug 22, 2012

I guess whether scribe needs more than one implementation comes down to whether user's need the choice between synchronous and asynchronous. Speaking only for myself, in these modern times with requirements for massive scale, I see no reason why a server handing a request should undertake any action which would block a service thread. We've moved beyond C10K (10,000 concurrent client) to C100K+, but a server where request threads block for tens or hundreds of milliseconds will quickly fall to a maximum of C1K (if it's lucky). From that perspective, if I had to choose "one true implementation" it would be asynchronous. However, there are existing scribe clients to consider, and the asynchronous pattern adds more complexity than some developers want to just "get something working". So, my approach was to support both models (without breaking backward-compatibility).

If what you're after is "dual mode" implementation where both synchronous and asynchronous take advantage of (depend on) apache commons http, I'm happy to take another cut at the changes. Or if what you want is something like a SPI (Service Provider Interface) or Factory model, where the implementation is "pluggable", I can definitely see utility in that approach. However, note that regardless of and SPI or Factory approach, the API for synchronous and asynchronous is similar but fundamentally different.

Just some thoughts to spur discussion.

I guess whether scribe needs more than one implementation comes down to whether user's need the choice between synchronous and asynchronous. Speaking only for myself, in these modern times with requirements for massive scale, I see no reason why a server handing a request should undertake any action which would block a service thread. We've moved beyond C10K (10,000 concurrent client) to C100K+, but a server where request threads block for tens or hundreds of milliseconds will quickly fall to a maximum of C1K (if it's lucky). From that perspective, if I had to choose "one true implementation" it would be asynchronous. However, there are existing scribe clients to consider, and the asynchronous pattern adds more complexity than some developers want to just "get something working". So, my approach was to support both models (without breaking backward-compatibility).

If what you're after is "dual mode" implementation where both synchronous and asynchronous take advantage of (depend on) apache commons http, I'm happy to take another cut at the changes. Or if what you want is something like a SPI (Service Provider Interface) or Factory model, where the implementation is "pluggable", I can definitely see utility in that approach. However, note that regardless of and SPI or Factory approach, the API for synchronous and asynchronous is similar but fundamentally different.

Just some thoughts to spur discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment