Closed JakeWharton closed 9 years ago
cc/ @adriancole @swankjesse
cc @benjchristensen
guys. this is great. looking forward to helping
Looks awesome. Love that Callbacks now become part of the real api, at compile-time.
Any examples of what error handling may look like in 2.0?
@austynmahoney I'm tempted to revert a bit back to pre-1.0 days with multiple callback methods. Something like this:
public interface Callback<T> {
void success(T result); // 2XX responses
void networkError(IOException e); // IOException from underlying input stream
void unexpectedError(Exception e); // Error from converter or internal Retrofit error.
void httpError(int status, ...); // non-2XX responses
}
What do you think?
Or maybe!
public interface Callback<T> {
void success(T result); // 2XX responses
}
public interface ErrorCallback {
void networkError(IOException e); // IOException from underlying input stream
void unexpectedError(Exception e); // Error from converter or internal Retrofit error.
void httpError(int status, ...); // non-2XX responses
class EmptyErrorCallback implements ErrorCallback {
@Override public void networkError(IOException e) {}
@Override public void unexpectedError(Exception e) {}
@Override public void httpError(int status, ...) {}
}
}
Just from what I see here, I'd say the second form seems preferable to me. A bit more granular control over errors vs success, and allows independent re-use of callbacks as needed.
I'd be really interested in seeing plans related to the support for different authentication mechanisms (i.e. oauth). Any thoughts on that?
Presumably the request and response interceptor/replacement would be enough to enable that. I have no plans to work on anything around oauth, digest, or the like myself. It will have to be contributed during the 2.0 development.
I would like to suggest that the Future
class has one single method to send the request, this being the get()
method. So instead of having a get()
method for synchronous operations and an addCallback()
(which as i understand it would send the request much like get()
) method for asynchronous ones i suggest having multiple overloaded get()
methods.
Synchronous request:
Response <Tweet> r = api.listTweets("JakeWharton");
List<Tweet> tweets = r.get();
Asynchronous request:
Response <Tweet> r = api.listTweets("JakeWharton");
r.get(new Callback<List<Tweet>>() {
@Override public void onSuccess(List<Tweet> response) { }
});
Asynchronous request (Observable):
Response <Tweet> r = api.listTweets("JakeWharton");
r.get(new ObservableCallback<Tweet>() {
@Override public void onNext(Tweet tweet) { }
// ...
});
I think this way of doing it would make it clearer what is happening for someone not familiar with the library. It also makes it clearer that you are done with the Future
instance.
Regarding the error handling i very much like what @JakeWharton suggested in his latest comment. I would however make a suggestion to remove the EmptyErrorCallback
base implementation. This encourages people to write more error prone code by not requiring handling of all the errors. If the user of retrofit would like to then that person can easily make a base implementation like EmptyErrorCallback
is their own codebase.
I like the new ErrorCallback
paradigm, it will be much easier for developers to know what actually happened and have less complex error handlers.
One thing I am worried about though is what the code will look like when you try to figure out the difference between a converter
and an internal
exception. Does anyone handle these two types of errors differently? Is there a recovery path that someone would want to go down if it was a converter error? I can't quite decide if this is important enough to separate converter and internal error into two different methods in the interface. Input on this?
I agree with @emilsjolander. The EmptyErrorCallback
will encourage devs to ignore errors completely. There isn't much extra work if they really want to do it themselves.
Just to make sure, 3xx responses are not getting passed into httpError()
right?
void httpError(int status, ...); // non-2XX responses
Another thing I'd like to see in 2.0 is generic error handling. Right now the handler you can add to RestAdapter.Builder
seems to only be called when an internal error occurs. I'd like to be able to give the builder an ErrorCallback
implementation that handles a subset of errors and propagates the ones it does not. It could possibly work like the touch event callback system on views.
class GenericErrorCallback implements ErrorCallback {
@Override public void networkError(IOException e) {...}
@Override public void unexpectedError(Exception e) {...}
@Override public void httpError(int status, ...) {
if(status == HTTP_401) {
// Do something
}
// We need something here like a boolean or return type so the error can be propagated to the ErrorCallback on the actual request.
}
}
I think ObservableCallback should be named Observer and the method that accepts it called subscribe and return a Subscription with a single method unsubscribe. This is nice and how rx does it.
Observer would have an onError(Throwable).
I'm not sure about the mutli ErrorCallback yet and would need to play with it.
I wonder if we could do a working design that could be published from a branch to sonatype snapshots repo. This could be an intentional throwaway in a different namespace. I wouldn't mind making 2 versions of my clients until we figure it out.
On Friday, August 9, 2013, Emil Sjölander wrote:
I would like to suggest that the Future class has one single method to send the request, this being the get() method. So instead of having a get() method for synchronous operations and an addCallback() (which as i understand it would send the request much like get()) method for asynchronous ones i suggest having multiple overloaded get() methods.
Synchronous request:
Response
r = api.listTweets("JakeWharton");List tweets = r.get(); Asynchronous request:
Response
r = api.listTweets("JakeWharton");r.get(new Callback<List >() { @Override public void onSuccess(List response) { }}); Asynchronous request (Observable):
Response
r = api.listTweets("JakeWharton");r.get(new ObservableCallback () { @Override public void onNext(Tweet tweet) { } // ...}); I think this way of doing it would make it clearer what is happening for someone not familiar with the library. It also makes it clearer that you are done with the Future instance.
Regarding the error handling i very much like what @JakeWhartonhttps://github.com/JakeWhartonsuggested in his latest comment. I would however make a suggestion to remove the EmptyErrorCallback base implementation. This encourages people to write more error prone code by not requiring handling of all the errors. If the user of retrofit would like to then that person can easily make a base implementation like EmptyErrorCallback is their own codebase.
— Reply to this email directly or view it on GitHubhttps://github.com/square/retrofit/issues/297#issuecomment-22379156 .
A 2.0 alpha release would be nice to play around with. This way we see how it feels using the new API in a real app.
Just hit a spot in an app I am working on where a 204 No Content
response is returned. A Void response in the callback would be useful here. How will this be handled in v2? The Callback<Void>
method suggested in #287 looks clean if you leave out the VoidResponse
stuff.
Absolutely agree callback void needs to be supported for this case.
On Friday, August 9, 2013, Austyn Mahoney wrote:
Just hit a spot in an app I am working on where a 204 No Content response is returned. A Void response in the callback would be useful here. How will this be handled in v2? The Callback
method suggested in #287https://github.com/square/retrofit/issues/287looks clean if you leave out the VoidResponse stuff. — Reply to this email directly or view it on GitHubhttps://github.com/square/retrofit/issues/297#issuecomment-22428727 .
Thoughts about Observer
Seems to work well enough for nice json lists from an RPC response (ex [ {}, {}, ..]
). A converter might look like this.
public void intoObserver(TypedInput body, Type type, Observer<? super Object> observer, AtomicBoolean subscribed) throws ConversionException {
JsonReader jsonReader = new JsonReader(body.in(), charset);
jsonReader.beginArray();
while (subscribed.get() && jsonReader.hasNext()) {
observer.onNext(fromJson(jsonReader, type));
}
}
So, this would emit each item in the array to the observer. Cool.. works.. saves reaction time, which could be seconds or longer.
On the other hand, "streaming" apis don't always have nice or similar json forms (think tweet blobs or newline). This could lead to confusing converter code. Further, anytime we touch or even think about IO, we'd have to think about the impact on IncrementalConverter, which if not widely used, could be wasted cycles.
This is compounded if folks interested in Observing things switch to message oriented or multiplexed protocols which have less need to send really long streams of things (which is what the incremental parsing optimizes for).
In short, I'm thinking just supporting Callback in 2.0 would be better than investing in Observer in our base model.
Dropped observable from the spec. Added error callback interface (still needs description).
I think the callback can be an abstract class:
public abstract class ResponseCallback<T> {
public abstract void success(T response);
public void networkError(IOException e) {
error(-1, e);
}
public void unexpectedError(Exception e) {
error(-1, e);
}
public void httpError(int status, ...) {
error(status, e);
}
public void error(int status, Exception e) {
log(...);
}
}
Though not as pure as an interface, this approach is ergonomic. You can handle all error types together, or you can handle each kind independently. This is the approach I'm considering for OkHttp. Whatever we do here, we should probably do similarly in OkHttp.
I think you should rename Response
to Call
and get
to execute
, with the symmetric access API @emilsjolander described above.
Call <Tweet> listTweetsCall = api.listTweets("JakeWharton");
// sync:
List<Tweet> tweets = listTweetsCall.execute();
// or async:
listTweetsCall.execute(new Callback<List<Tweet>>() {
@Override public void onSuccess(List<Tweet> response) { }
});
I prefer execute
over get
because it's clear that that's when the work is kicked off by the executor thread. With non-action method names like get()
or setCallback
it's unclear when the executor starts executoring. (And we shouldn't kick off the work in a background thread until we know that the request is async.)
yeah I think this would keep complexity at a minimum and optimizes for a single signaling approach: ex. when in callback mode, you'd likely also want errors as a callback. Visa-versa: when working in sync-mode, you won't have an error callback.
Q: when okhttp has a callback interface, with this be the only interface which one would build sync on-top-of? or would it be that there would be 2 ways in: sync or async?
@adriancole now that I've seen this simple+powerful API, I want to mirror it exactly in OkHttp! But instead of T
OkHttp's Response object would have headers and a streamable body.
Possibly overengineering, but having access to the duration when a timeout occurred is really helpful when dealing with flaky networks or services wdyt about:
in either case, could be punted to a member on an exception
I'm not sure Call
is descriptive enough. This is a Request
to a server, not just a method call. The Request
naming convention is also used elsewhere in Retrofit and other libraries (e.g. HttpRequest
, RequestInterceptor
).
(I like Call
cause it's a request and a response.)
Call
is nice. I couldn't think of anything which is why I left it as Response
.
@austynmahoney Using Response
before was definitely a misnomer because having methods like cancel()
or retry()
make no sense on a response. It's definitely a request/response pair that's wrapped up in one object which is hard to name.
I'm not opposed to an abstract class. It's how we approach the problem internally for the current Callback
interface. I want to ensure we never sweep failures under the rug and that they get appropriate attention by defaulting to throwing exceptions.
What do you think about this:
public abstract class ResponseCallback<T> {
public abstract void success(T response);
public void networkError(IOException e) {
error(ErrorType.NETWORK, -1, e);
}
public void unexpectedError(Exception e) {
error(ErrorType.UNEXPECTED, -1, e);
}
public void httpError(int status, ...) {
error(ErrorType.HTTP, status, e);
}
public void error(ErrorType errorType, int status, Exception e) {
throw new RuntimeException(.., e);
}
public enum ErrorType { NETWORK, UNEXPECTED, HTTP }
}
+1 to ResponseCallback<T>
as defined above. The only one I wonder about is UNEXPECTED
, since it's awkward to handle it. Doesn't UNEXPECTED
imply a programming bug? I try to avoid writing code to handle programming bugs; preferring instead to crash the app.
The primary use case for unexpected is an exception thrown during processing the response. For example, if I pass HTML or a proto to GsonConverter
it's going to throw an exception. Ideally this would never cover underlying programmer errors... not that we ever make any!
Perhaps rename to processingError
?
A use case to consider for v2: RequestInterceptor
was changed to be invoked at the time of method invocation rather than at the time of request execution. This prevents you from doing any kind of semi-slow work (read File
, check AccountManager
, etc) for every request on Android since StrictMode
will blow up in your face.
@JakeWharton Agreed. as much as possible, we should prepare the request eagerly. I love the idea of more errors up front. We could consider migrating methodDetails.init()
and the prep steps inside the RequestBuilder
ctor into a per method cache.
w/ annotation processing + writing, we could write classes for what's now in RequestBuilder.setArguments and RequestBuilder *. These new classes could, for example, employ the type system if helpful.
Ex. instead of
requestBuilder = new RequestBuilder(converter, methodDetails);
we could extract this to load from a javawriter or a reflection fallback
requestBuilder = newRequestBuilder(methodKey);
In this case, a javawriter RequestBuilder could be written to implement IdempotentRequestBuilder
or ReadOnlyRequestBuilder
, which could help in dispatch.
Something to consider wrt eager request interceptor. Depending on retry behavior, a session or otherwise temporal header could be valid for the first request, but not the retry. When the interceptor is dynamic, this could change between tries. Same thing follows for the URL ex it could change on the second try to avoid the first server.
In other words, if a request is retried on temporary failure and the params added to the request are time or host sensitive, lazy invocation of the request interceptor could be desirable.
I would argue for the opposite behavior! I think it's too soon for that decision, however.
FWIW, I think the argument is stronger for eager vs lazy. This is just me capturing the other side.
Perhaps relegated to a pattern section, I think we should keep link header pagination in mind for 2.0
I am interested in discussing HATEOS API consumption as a possibility for v2 (initial discussion: https://github.com/square/retrofit/issues/333).
Is there currently a public branch for 2.0 somewhere?
No. Nobody is working on it at this time.
Jake Wharton http://about.me/jakewharton
On Tue, Dec 17, 2013 at 1:40 PM, Nathan Schwermann <notifications@github.com
wrote:
Is there currently a public branch for 2.0 somewhere?
— Reply to this email directly or view it on GitHubhttps://github.com/square/retrofit/issues/297#issuecomment-30793999 .
Do you envision the invokeRequest method of the RestAdapter always returning a Future instead of an Object in this spec? I've been contemplating implementing a Client using AndroidAsync with this library but it currently seems to always force the client calls to be synchronous under the covers, I'm not seeing a way to implement Async Callbacks for my client without changing the implementation to the RestAdapter.
Underlying implementation will always be synchronous. The async behavior (be it simple Callback
, Future
, Observable
, etc.) will come as a "plugin", of sorts.
Updated with a brief description of the plugin system which I refer to as extensions, for now.
At the moment, in the current stable version, what is the best canceling mechanism? https://plus.google.com/104291479989053863842/posts/P3fVcKuVXbQ
This is not the place for that discussion. You've already posted on another issue and the Google+ Community. On Jan 24, 2014 4:34 PM, "Juan Jose Alonso" notifications@github.com wrote:
At the moment, in the current stable version, what is the best canceling mechanism? https://plus.google.com/104291479989053863842/posts/P3fVcKuVXbQ
Reply to this email directly or view it on GitHubhttps://github.com/square/retrofit/issues/297#issuecomment-33275021 .
You are right. Sorry. El 25/01/2014 01:44, "Jake Wharton" notifications@github.com escribió:
This is not the place for that discussion. You've already posted on another issue and the Google+ Community. On Jan 24, 2014 4:34 PM, "Juan Jose Alonso" notifications@github.com wrote:
At the moment, in the current stable version, what is the best canceling mechanism? https://plus.google.com/104291479989053863842/posts/P3fVcKuVXbQ
Reply to this email directly or view it on GitHub< https://github.com/square/retrofit/issues/297#issuecomment-33275021> .
— Reply to this email directly or view it on GitHubhttps://github.com/square/retrofit/issues/297#issuecomment-33275469 .
It may be best to actually yield control of parsing to the extension and provide it with an API to help with reflection. This would allow extensions to (among other things) easily parse return types for Retrofit annotations.
The rationale for parsing return types is to support zero-arg factories for function types. It's a pattern that makes it much easier for app developers to build higher-order constructs on top of Retrofit.
Sample/sketch:
interface RxGithub {
GetRepos getRepos();
GetCollaborators getCollaborators();
interface GetRepos extends Func1<UserId, List<Repo>> {
List<Repo> call(@Path("user") UserId user);
}
interface GetCollaborators extends Func1<RepoId, List<User>> {
List<User> call(@Path("repo") RepoId repo);
}
}
WRT the response callback, it's probably worthwhile to distinguish errors that occur while preparing the request from errors that occur while processing the response. They may not be programming errors-- for instance, if an interceptor itself performs I/O, it may fail.
Do I understand it right, that after introducing Call
interface, how request is actually executed (I mean what thread or what executor) is an implementation detail of an extension that supports this interface? This also means that RestAdatper.Builder
won't have setExecutors
method.
Will it be allowed to override the default extension for Call
interface, or extensions will always have to introduce a new type?
@roman-mazur Unsure. I can see value in replacing the built-in Call
type's extension.
This is a living spec for what will be v2 of Retrofit.
Goals
RestAdapter
or generated interface instance.Changes
Annotation Processors
An annotation processor will be embedded inside the core artifact for compile-time verification of REST API interface declarations.
A second annotation processor will be provided as a separate artifact for full code generation of a class implementation of each API interface.
The
RestAdapter
will always do a read-through cached lookup for the generated classes since it has no knowledge of whether the code-gen processor was used and we don't want to place the burden on the caller either.Request Object
All interface declarations will be required to return an object through which all interaction will occur. The behavior of this object will be similar to a
Future
and will be generic typed (T
) for the success response type (ref: #231).Callers can synchronously call
.execute()
which will return typeT
. Exceptions will be thrown for any error, network error, unsuccessful response, or unsuccessful deserialization of the response body. While these exceptions will likely extend from the same supertype, it's unclear as to whether that supertype should be checked or unchecked.Callers can also supply callbacks to this object for asynchronous notification of the response. The traditional
Callback<T>
of the current version of Retrofit will be available. One change will be that the error object passed tofailure
will not be the same exception as would be thrown in synchronous execution but rather something a bit more transparent to the underlying cause.TODO describe error handling
The call object is also an obvious place for handling the retry and cancelation of requests. Both are a simple, no-args method on the object which can only be called at appropriate times.
cancel()
is a no-op after the response has been received. In all other cases the method will set any callbacks tonull
(thus freeing strong references to the enclosing class if declared anonymously) and render the request object dead. All future interactions with the request object will throw an exception. If the request is waiting in the executor itsFuture
will be cancelled so that it is never invoked.retry()
will re-submit the request onto the backing executor without passing through any of the mutating pipeline described above. Retrying a request is only available after a network error or 5XX response. Attempting to retry a request that is currently in flight, after a non-5XX response, after an unexpected error, or after callingcancel()
will throw an exception.Extension System
In order to facilitate libraries which offer semantics on both synchronous and asynchronous operations without requiring a wrapper, we will introduce a system which we will tentatively refer to as extensions. An extension instance will be registered with the
RestAdapter
and associate itself as handling an explicit interface method return type.By default, Retrofit will install its own extension which handles the aforementioned
Call
type for both synchronous and asynchronous request execution.The current (albeit experimentally denoted) RxJava integration will also be provided as an opt-in extension. This extension will register itself as handling the
Observable
type which will allow the declaration of interfaces which return this type.And more...
TODO!