* It's not entirely clear whether an [Async]Resource is meant to be single use or reusable. this.data first is the request body, then the response body. Same with this.headers. * HTTP status code and headers are figured out in the onStopRequest handler which means there's no way for _onProgress consumers to know whether the request has actually succeeded. This means right now we're trying to read JSON records from, say, a 404 error page, and then *later* realize it's a 404. We should be able to figure that out earlier. * An _onProgress handler that throws allows the whole request to go through. With AsyncResource, the callback gets an NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS error passed in. Resource converts that into an actual exception and throws it. It would be better if the request was aborted and the original error was made available to the calling code. * There's no way to abort an ongoing request. In fact, even the timer-based abort doesn't *actually* abort the request. It just sets all the handlers to no-ops.

I have an alternative implementation.

(In reply to comment #1) > I have an alternative implementation. Tee hee.

This implements RESTResource and related components. Inspired by [Async]Resource, but separate and without the API flaws. Well, at least *less* flaws :p

This changes our only consumer of AsyncResource so far -- JPAKEClient -- over to RESTResource.

Comment on attachment 544342 [details] [diff] [review] Implement RESTResource etc. (v1) Review of attachment 544342 [details] [diff] [review]: ----------------------------------------------------------------- Grammar nit: *fewer* flaws :P ::: services/sync/modules/rest.js @@ +18,5 @@ > + * Portions created by the Initial Developer are Copyright (C) 2011 > + * the Initial Developer. All Rights Reserved. > + * > + * Contributor(s): > + * Philipp von Weitershausen <philipp@weitershausen.de> Looks like some of this file includes changes I added in 32c8658040b775dc6fff4e903305ceaac6d57908 in alder, and the UA stuff from existing Sync code, so I suppose I need to appear here as one of the guilty parties! @@ +43,5 @@ > +Cu.import("resource://services-sync/constants.js"); > + > +const EXPORTED_SYMBOLS = ["RESTRequest", "SyncStorageRequest"]; > + > +const STORAGE_REQUEST_TIMEOUT = 300000; I had presumed that this was here to allow the whole file to be lifted out of Sync and into some platform component. However, STORAGE_REQUEST_TIMEOUT is only used by SyncStorageRequest, so it seems to me that it should be named SYNC_STORAGE_REQUEST_TIMEOUT, and live in constants.js. It wouldn't be lifted with RESTRequest. @@ +48,5 @@ > + > +/** > + * Single use HTTP requests to RESTish resources. > + * > + * Examples: Hooray for good docs with code examples! @@ +67,5 @@ > + * } > + * processData(this.response.body); > + * }); > + * > + * (2) Quick PUT request (data is automatically JSONified) Nit: *non-string data*. (I will ignore the plurality of data. Curse you, common English usage! :P) @@ +101,5 @@ > + this.status = this.NOT_SENT; > + > + // If we don't have an nsIURI object yet, make one. This will throw if > + // 'uri' isn't a valid URI string. > + if (!(uri instanceof Ci.nsIURI)) { This should probably go into a setter for the uri property. Otherwise you're going to see this broken pattern: let r = new RESTRequest("http://foo"); r.uri = "http://foo/bar?baz=noo"; @@ +161,5 @@ > + COMPLETED: 4, > + ABORTED: 8, > + > + /** > + * Request timeout (in seconds). Whole seconds? @@ +205,5 @@ > + * @return the request object. > + */ > + get: function get(onComplete, onProgress) { > + this.dispatch("GET", null, onComplete, onProgress); > + return this; Can we just get dispatch() to return this, and change all of these to return this.dispatch(...) ? @@ +265,5 @@ > + */ > + abort: function abort() { > + if (this.status != this.SENT && this.status != this.IN_PROGRESS) { > + throw "Can only abort a request that has been sent."; > + } I'm not sure this is 100% correct. IN_PROGRESS becomes the state in onStartRequest. Imagine this: * Caller creates and sends a RESTRequest. Starts a local timer of some sort, or waits for an event, or... * Timer fires, they want to cancel the RESTRequest. If the request timeout is longer than the duration, we could still be in the NOT_SENT state. abort will throw. The caller has no reasonable way to reliably abort! Another scenario is that a redirect is encountered: the nsIChannelEventSink redirect callbacks are invoked before onStartRequest. I suggest that there ought to be another state, perhaps DISPATCHED, which means "you called get/post/etc., but the onStartRequest handler has not yet been called". Aborting should be possible in this state. We might do that by augmenting or replacing the onStartRequest handler to check an "I should abort" flag. Alternatively, adjust this method to only try to cancel the channel when IN_PROGRESS or SENT. @@ +280,5 @@ > + /*** Implementation stuff ***/ > + > + dispatch: function dispatch(method, data, onComplete, onProgress) { > + if (this.status != this.NOT_SENT) { > + throw "Request has already been sent!"; Related: this means that let r = new RESTRequest("http://foo"); r.get(function () {}); r.get(function () {}); won't error, because the onStartRequest callback will fire after the second get() call. You need to change state right here in dispatch, not just in a callback. @@ +317,5 @@ > + if (typeof data != "string") { > + data = JSON.stringify(data); > + } > + > + if (this._log.level <= Log4Moz.Level.Trace) { You're checking for Trace, then calling .debug inside. Split out? @@ +346,5 @@ > + */ > + delayTimeout: function delayTimeout() { > + if (this.timeout) { > + Utils.namedTimer(this.abortTimeout, this.timeout * 1000, this, > + "timeoutTimer"); Wait, doesn't this mean that I can't start multiple concurrent requests without screwing up each's timeout? @@ +409,5 @@ > + // XPCOM borders while preserving the status code. > + if (!Components.isSuccessCode(statusCode)) { > + let message = Components.Exception("", statusCode).name; > + let error = Components.Exception(message, statusCode); > + this.onComplete(error); I personally favor setting this.onComplete to null after it's been invoked. That avoids the remote possibility of a callback being invoked twice. @@ +584,5 @@ > + dispatch: function dispatch(method, data, onComplete, onProgress) { > + // Compose a UA string fragment from the various available identifiers. > + if (Svc.Prefs.get("sendVersionInfo", true)) { > + let ua = this.userAgent + Svc.Prefs.get("client.type", "desktop"); > + this.setHeader("User-Agent", ua); It seems needlessly inefficient to require setHeader to lowercase the same constant strings each time, no? @@ +591,5 @@ > + // Set the BasicAuth header. > + let id = ID.get("WeaveID"); > + if (id) { > + let auth_header = "Basic " + btoa(id.username + ':' + id.passwordUTF8); > + this.setHeader("Authorization", auth_header); Same. ::: services/sync/tests/unit/test_restrequest.js @@ +20,5 @@ > + do_check_throws(function() { > + new RESTRequest("an invalid URI"); > + }, Cr.NS_ERROR_MALFORMED_URI); > + run_next_test(); > +}); Can we add a test either for setting .uri, or verifying that setting .uri is not permitted?

(In reply to comment #5) > Looks like some of this file includes changes I added in > 32c8658040b775dc6fff4e903305ceaac6d57908 in alder, and the UA stuff from > existing Sync code, so I suppose I need to appear here as one of the guilty > parties! Good point. I'll also add Dan and Anant since they wrote the original Resource stuff. > @@ +43,5 @@ > > +Cu.import("resource://services-sync/constants.js"); > > + > > +const EXPORTED_SYMBOLS = ["RESTRequest", "SyncStorageRequest"]; > > + > > +const STORAGE_REQUEST_TIMEOUT = 300000; > > I had presumed that this was here to allow the whole file to be lifted out > of Sync and into some platform component. Well, not necessarily the whole file, but large portions of it. > However, STORAGE_REQUEST_TIMEOUT is only used by SyncStorageRequest, so it > seems to me that it should be named SYNC_STORAGE_REQUEST_TIMEOUT, and live > in constants.js. It wouldn't be lifted with RESTRequest. We don't need to add everything to constants.js, only constants that need to be available from more than one module. This one doesn't, so I think we should keep it a module-level one. > > + this.status = this.NOT_SENT; > > + > > + // If we don't have an nsIURI object yet, make one. This will throw if > > + // 'uri' isn't a valid URI string. > > + if (!(uri instanceof Ci.nsIURI)) { > > This should probably go into a setter for the uri property. Otherwise you're > going to see this broken pattern: > > let r = new RESTRequest("http://foo"); > r.uri = "http://foo/bar?baz=noo"; AsyncResource does it the magic setter thing. I don't like it and explicitly opted against it. Setters and getters that aren't symmetric are very confusing IMHO. Overloaded constructors, on the other hand, is a pretty common pattern. So if you want to set the 'uri' attribute after creating a request, you must set it to an nsIURI object. I'll update the comment for it, and document the two options for the constructor. > > + /** > > + * Request timeout (in seconds). > > Whole seconds? Well, you can use decimals to get up to millisecond granularity if you want. I just wanted to keep the number of zeros down. And now that you mention it, I forgot that the timeout const is still in milliseconds! > > + */ > > + abort: function abort() { > > + if (this.status != this.SENT && this.status != this.IN_PROGRESS) { > > + throw "Can only abort a request that has been sent."; > > + } > > I'm not sure this is 100% correct. IN_PROGRESS becomes the state in > onStartRequest. Imagine this: > > * Caller creates and sends a RESTRequest. Starts a local timer of some sort, > or waits for an event, or... > * Timer fires, they want to cancel the RESTRequest. If the request timeout > is longer than the duration, we could still be in the NOT_SENT state. abort > will throw. I think you're missing something. dispatch() sets the status to SENT, so in your scenario it will never not be in NOT_SENT. > Another scenario is that a redirect is encountered: the nsIChannelEventSink > redirect callbacks are invoked before onStartRequest. > > I suggest that there ought to be another state, perhaps DISPATCHED, which > means "you called get/post/etc., but the onStartRequest handler has not yet > been called". That's what SENT means. Do you want to rename it to DISPATCHED? > > + /*** Implementation stuff ***/ > > + > > + dispatch: function dispatch(method, data, onComplete, onProgress) { > > + if (this.status != this.NOT_SENT) { > > + throw "Request has already been sent!"; > > Related: this means that > > let r = new RESTRequest("http://foo"); > r.get(function () {}); > r.get(function () {}); > > won't error, because the onStartRequest callback will fire after the second > get() call. You need to change state right here in dispatch, not just in a > callback. Again, I think you'll find that's not true because I have the SENT state. See my test_get() testcase (the do_check_throws test at the end of the function). > > + */ > > + delayTimeout: function delayTimeout() { > > + if (this.timeout) { > > + Utils.namedTimer(this.abortTimeout, this.timeout * 1000, this, > > + "timeoutTimer"); > > Wait, doesn't this mean that I can't start multiple concurrent requests > without screwing up each's timeout? Huh?!? Each request gets its own timer. Will address all your other comments. Thanks for the thorough review!

(In reply to comment #6) > We don't need to add everything to constants.js, only constants that need to > be available from more than one module. This one doesn't, so I think we > should keep it a module-level one. Fine by me. > So if you want to set the 'uri' attribute after creating a request, you must > set it to an nsIURI object. I'll update the comment for it, and document the > two options for the constructor. Documentation works for me. > Well, you can use decimals to get up to millisecond granularity if you want. > I just wanted to keep the number of zeros down. And now that you mention it, > I forgot that the timeout const is still in milliseconds! :) Please document the allowed values (e.g., "in seconds, up to millisecond granularity"). I've encountered too many APIs that don't allow fractional seconds for this kind of thing. > I think you're missing something. dispatch() sets the status to SENT, so in > your scenario it will never not be in NOT_SENT. Yes, I missed that. Fail. I even looked for it! I think I wasn't expecting it to be so far down dispatch(). Glad you pre-empted everything I thought of :D > That's what SENT means. Do you want to rename it to DISPATCHED? SENT is fine. > Huh?!? Each request gets its own timer. Oh, duh, the named timer is attached to the object. I have a case of the dumb. > Will address all your other comments. Thanks for the thorough review! Thanks for the patience :P

Comment on attachment 544344 [details] [diff] [review] Use RESTResource in JPAKEClient (v1) Review of attachment 544344 [details] [diff] [review]: ----------------------------------------------------------------- http://etler.com/clapping.gif ::: services/sync/modules/jpakeclient.js @@ +323,1 @@ > if (this._etag) While you're here, can I lend you these? {} ::: services/sync/tests/unit/test_jpakeclient.js @@ +126,5 @@ > > // Ensure PSM is initialized. > Cc["@mozilla.org/psm;1"].getService(Ci.nsISupports); > > + // Simulate Sync setup with a credentials in place. We want to make s/a cred/cred

Address rnewman's review comments.

Address rnewman's review comments.

http://hg.mozilla.org/services/services-central/rev/58c29d47f052 http://hg.mozilla.org/services/services-central/rev/ff8feff09e04

Comment on attachment 544373 [details] [diff] [review] Implement RESTResource etc. (v2) Review of attachment 544373 [details] [diff] [review]: ----------------------------------------------------------------- FWIW, I don't think this is good enough yet to be taken into toolkit or network as-is. ::: services/sync/modules/rest.js @@ +100,5 @@ > + * return; > + * } > + * // Process body data and reset it so we don't process the same data twice. > + * processIncrementalData(this.response.body); > + * this.response.body = ""; Why are you setting this to the empty string? @@ +160,5 @@ > + * Flag to indicate the status of the request. > + * > + * One of NOT_SENT, SENT, IN_PROGRESS, COMPLETED, ABORTED. > + */ > + status: null, Shouldn't this default to `NOT_SENT`? @@ +174,5 @@ > + * up to millisecond granularity.) > + * > + * 0 for no timeout. > + */ > + timeout: null, Shouldn't this default to `0`, not `null`? @@ +182,5 @@ > + * timeouts. > + * > + * @param error > + * Error that occurred while making the request, null if there > + * was no error. More details on what the error object looks like would be nice here. @@ +269,5 @@ > + * Abort an active request. > + */ > + abort: function abort() { > + if (this.status != this.SENT && this.status != this.IN_PROGRESS) { > + throw "Can only abort a request that has been sent."; You should really construct an error object with `Components.Exception` and not throw a string. @@ +285,5 @@ > + /*** Implementation stuff ***/ > + > + dispatch: function dispatch(method, data, onComplete, onProgress) { > + if (this.status != this.NOT_SENT) { > + throw "Request has already been sent!"; same here @@ +301,5 @@ > + let channel = Services.io.newChannelFromURI(this.uri, null, null) > + .QueryInterface(Ci.nsIRequest) > + .QueryInterface(Ci.nsIHttpChannel); > + this.channel = channel; > + channel.loadFlags |= this.loadFlags; It would be useful to warn on any invalid channel flags here. Probably also useful to log the flags you are setting. @@ +307,5 @@ > + > + // Set request headers. > + let headers = this._headers; > + for (let key in headers) { > + if (key == 'authorization') { magic strings should be pulled into constants, FWIW @@ +429,5 @@ > + } > + > + delete this._inputStream; > + > + this.onComplete(null); Why are you explicitly passing `null` here instead of letting it be `undefined`? @@ +470,5 @@ > + > + notifyCertProblem: function notifyCertProblem(socketInfo, sslStatus, targetHost) { > + this._log.warn("Invalid HTTPS certificate encountered!"); > + // Suppress invalid HTTPS certificate warnings in the UI. > + // (The request will still fail.) You should have a test for this. I don't see one.

Thanks, sdwilsh! Filed bug 671130 for your comments since we want to wrap up this bug.

http://hg.mozilla.org/mozilla-central/rev/58c29d47f052 http://hg.mozilla.org/mozilla-central/rev/ff8feff09e04