Add ability to generalize over a HTTP request being async or not. #347

tcard · 2017-08-31T20:22:26Z

This was something I had done in the push, but is independent of it.

Move AsyncHttp to CallbackfulHttp, and instead of inheriting from
ThreadPoolExecutor, take any Executor as a parameter. Then, have
both AsyncHttp and SyncHttp inheriting from it. AsyncHttp injects a
ThreadPoolExecutor, as before, while SyncHttp injects an Executor
that just executes Runnables right away, in the current thread.
Thus, SyncHttp exposes an “asynchronous” (callbackful) interface
while behaving synchronously.

By using CallbackfulHttp, we can abstract over whether an operation
is executed synchronously or asynchronously.

To make this more user-friendly, AnySyncHttp wraps both a SyncHttp
and an AsyncHttp, and lets you specify how to execute (class Execute) a
request (class Request) and then getting its result synchronously or
asynchronously (in another thread), as a method call result or passed
to a callback, with the sync and async methods respectively.

This design should make the Http class obsolete, once/if all the code
that uses it is ported. It could also be left as a proxy to SyncHttp.

(I did this after I renamed something in a foo method but forgot to
do the same in a fooAsync method, causing a bug that was reported by
a user.)

As an example, I've refactored Channel.publish(Message[]). Luckily, I've found an instance of the class of errors this is supposed to fix: RTL6g3 was implemented for publish, but not for publishAsync. Now, as they share their code, is implemented by both.

Move AsyncHttp to CallbackfulHttp, and instead of inheriting from ThreadPoolExecutor, take any Executor as a parameter. Then, have both AsyncHttp and SyncHttp inheriting from it. AsyncHttp injects a ThreadPoolExecutor, as before, while SyncHttp injects an Executor that just executes Runnables right away, in the current thread. Thus, SyncHttp exposes an “asynchronous” (callbackful) interface while behaving synchronously. By using CallbackfulHttp, we can abstract over whether an operation is executed synchronously or asynchronously. To make this more user-friendly, AnySyncHttp wraps both a SyncHttp and an AsyncHttp, and lets you specify how to execute (class Execute) a request (class Request) and then getting its result synchronously or asynchronously (in another thread), as a method call result or passed to a callback, with the `sync` and `async` methods respectively. This design should make the Http class obsolete, once/if all the code that uses it is ported. It could also be left as a proxy to SyncHttp. (I did this after I renamed something in a `foo` method but forgot to do the same in a `fooAsync` method, causing a bug that was reported by a user.)

Plus, fix bug by which RTL6g3 was not implemented for the async version.

mattheworiordan

Few small comments in regards to naming & code comments, but otherwise looks good to me. Caveat is that I don't really know the Java code base that well so can't really comment on the overall design and how it fits in.

mattheworiordan · 2017-09-01T11:12:48Z

lib/src/main/java/io/ably/lib/http/AnySyncHttp.java

+ * Created by tcard on 28/04/2017.
+ */
+
+public class AnySyncHttp {


Should we perhaps add a brief comment as to the purpose of this class for future us?

mattheworiordan · 2017-09-01T11:14:08Z

lib/src/main/java/io/ably/lib/http/CallbackfulHttp.java

+ * Created by tcard on 31/8/17.
+ */
+
+public class CallbackfulHttp<Executor extends java.util.concurrent.Executor> {


What is CallbackfulHttp i.e. I am not clear on what the ful after Callback implies. Perhaps a high level comment on what this does is needed.

paddybyers · 2017-09-01T12:12:33Z

Overall I like this as a way of rationalising the duplication we had but my reservations at the moment are:

names .. I don't like CallbackfulHttp, AnySyncHttp, publishAnySync etc - I'm trying renaming a few things to see how it looks.
I would really like to see the migration from Http to SyncHttp completed and AblyRest.http removed. That can be part of a follow-up PR but we should do it now instead of later.
I don't like the passing of results back in array members in

ably-java/lib/src/main/java/io/ably/lib/http/AnySyncHttp.java

Line 30 in e60c31c

final Object[] result = new Object[1];

, and especially the fact that it's an untyped Object and there need to be unchecked casts. There's surely enough generics to avoid this.

I'll be back with a few naming suggestions.

tcard · 2017-09-01T13:53:41Z

If we get rid of Http, CallbackfulHttp can be just Http (or AbstractHttp for extra Javaness). CallbackfulHttp is named that way as opposed to "callbackless" Http, because that's the difference between them.

I also don't love AnySyncHttp. I've thought about HttpProxy, because it's just a GoF proxy for AsyncHttp and SyncHttp, but it could be confused with an HTTP proxy in the network sense. HttpRequester maybe?

tcard · 2017-09-01T15:08:14Z

f782a9e addresses @paddybyers 's point 3.

paddybyers · 2017-09-01T15:18:29Z

lib/src/main/java/io/ably/lib/http/AnySyncHttp.java

+        this.syncHttp = syncHttp;
+    }
+
+    public static class Request<Result> {


I don't see why this is static and yet it has a anySyncHttp member. I think you can remove that member and remove the static declaration.

Done at 08f3c98.

paddybyers · 2017-09-01T15:20:09Z

lib/src/main/java/io/ably/lib/http/AnySyncHttp.java

+        }
+
+        public Result sync() throws AblyException {
+            final SyncExecuteResult<Result> result = new SyncExecuteResult<>();


Yes, I was looking at just having syncResult and syncErr members in the Result type, instead of a new type to hold them, but what you have is a bit cleaner.

paddybyers · 2017-09-01T15:29:45Z

OK, I think we're heading towards a messy state where the existing Http class is a mixture of various configuration options, some ancillary classes (RequestBody etc) and some of the request primitives that are used elsewhere in the API. I think we should be refactoring that into something like HttpOptions or HttpConfig which contains the options-derived state (hosts etc); and separate out the ancillary types into a separate class (or an http.types package) and the lower primitives can either be removed to eliminate layer duplication, or we can rename that class as HttpImpl or HttpEngine or something.

That will potentially free up the Http name for something that actually represents the API that the rest of the library uses.

Doing that refactor isn't trivial because we also have a lot of tests that rely on those primitive APIs.

Anyway, if we're not doing that yet, how about renaming things as:

CallbackfulHttp -> AbstractHttpExecutor

AsyncHttp, SyncHttp can stay the same or be renamed AsyncHttpExecutor, SyncHttpExecutor

AnySyncExecutor -> HttpExecutor maybe.

publishAnySync -> executePublish or publishImpl maybe.

paddybyers · 2017-09-01T15:31:59Z

lib/src/main/java/io/ably/lib/http/AnySyncHttp.java

+    }
+
+    public interface Execute<Result> {
+        public void execute(CallbackfulHttp http, Callback<Result> callback) throws AblyException;


I've getting a raw types warning here and elsewhere; it should be CallbackfulHttp<? extends Executor> http.

The old Http class has been split up. We have now: * HttpCore implements the core primitives to do Ably-ready requests, ie. HTTP requests on the current thread that implement Ably auth, know how to reauthorize, and interpret Ably's error responses as ErrorInfos. It also has ancillary inner classes like Response, BodyHandler. * HttpScheduler, that implements operations on top of a HttpCore scheduled to an Executor. The API is asynchronous, but the underlying Executor may make its behaviour synchronous. We have both SyncHttpScheduler and AsyncHttpScheduler subclasses. * Http, which offers a higher level API to SyncHttpScheduler and AsyncHttpScheduler to help expose a synchronous or asynchronous API out of HTTP operations without duplicating code. * HttpHelpers, for helper methods used primarily in tests. It's also used in some non-test places that don't need the sync/async unification so I didn't bother converting them to use Http, which would make them more complex. * HttpConstants to hold constants like method names, header names, etc. PaginatedQuery/Result and AsyncPaginatedQuery/Result have now unified implementations too. Now both wrap BasePaginatedQuery/Result. To make this user-friendly some internal bridging classes boilerplate had to be added, unfortunately, but the end result should be simple enough. Tests have not been converted yet.

tcard · 2017-09-05T12:34:18Z

@paddybyers PTAL. I should have split 080a386 in at least a couple of steps but unfortunately I didn't, and since renaming and refactoring went hand in hand it's impossible to do after the fact.

See the comments in 080a386 's commit message.

paddybyers · 2017-09-20T07:13:29Z

This looks very good, thanks.

Are you planning to update the tests in this PR?

tcard · 2017-09-20T22:30:53Z

@paddybyers Yes, I'll do that tomorrow.

tcard · 2017-09-28T17:35:45Z

There are some test that are failing, but I took a look and they didn't seem related to these changes. I'm raising an issue to fix them and merging this PR.

tcard added 3 commits August 31, 2017 22:06

Add CompletionListener.ToCallback that converts to Callback<Void>.

dba5573

Refactor Channel.publish(Message[]) to use AnySync.

e60c31c

Plus, fix bug by which RTL6g3 was not implemented for the async version.

tcard requested a review from paddybyers August 31, 2017 20:22

mattheworiordan reviewed Sep 1, 2017

View reviewed changes

AnySyncHttp: use a class instead of the array hack to capture results.

f782a9e

paddybyers reviewed Sep 1, 2017

View reviewed changes

AnsySnyHttp: make Request subclass non-static.

08f3c98

paddybyers reviewed Sep 1, 2017

View reviewed changes

Adapt tests to HTTP refactor.

b31db28

tcard merged commit b3c8862 into master Sep 28, 2017

tcard deleted the any-sync-http branch September 28, 2017 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to generalize over a HTTP request being async or not. #347

Add ability to generalize over a HTTP request being async or not. #347

tcard commented Aug 31, 2017

mattheworiordan left a comment

mattheworiordan Sep 1, 2017

mattheworiordan Sep 1, 2017

paddybyers commented Sep 1, 2017

tcard commented Sep 1, 2017

tcard commented Sep 1, 2017

paddybyers Sep 1, 2017

tcard Sep 1, 2017

paddybyers Sep 1, 2017

paddybyers commented Sep 1, 2017

paddybyers Sep 1, 2017

tcard commented Sep 5, 2017

paddybyers commented Sep 20, 2017

tcard commented Sep 20, 2017

tcard commented Sep 28, 2017

Add ability to generalize over a HTTP request being async or not. #347

Add ability to generalize over a HTTP request being async or not. #347

Conversation

tcard commented Aug 31, 2017

mattheworiordan left a comment

Choose a reason for hiding this comment

mattheworiordan Sep 1, 2017

Choose a reason for hiding this comment

mattheworiordan Sep 1, 2017

Choose a reason for hiding this comment

paddybyers commented Sep 1, 2017

tcard commented Sep 1, 2017

tcard commented Sep 1, 2017

paddybyers Sep 1, 2017

Choose a reason for hiding this comment

tcard Sep 1, 2017

Choose a reason for hiding this comment

paddybyers Sep 1, 2017

Choose a reason for hiding this comment

paddybyers commented Sep 1, 2017

paddybyers Sep 1, 2017

Choose a reason for hiding this comment

tcard commented Sep 5, 2017

paddybyers commented Sep 20, 2017

tcard commented Sep 20, 2017

tcard commented Sep 28, 2017