Skip to content

Commit

Permalink
Implement XHR timeout for Android and IOS natively.
Browse files Browse the repository at this point in the history 
Summary:
Opening this in a separate PR but the discussion can be viewed on #4832.

Basically, this is a native implementation and is a bit more elegant. The consensus on my previous PR was that it should be done natively rather than in JS.

There's now no maximum valid timeout value and a timeout of 0 will never time out.

ontimeout isn't implemented (yet) in this PR.

cc nicklockwood ide philikon
Closes #5038

Reviewed By: svcscm

Differential Revision: D2838743

Pulled By: nicklockwood

fb-gh-sync-id: 774f864ac35082bf522f7665f4311bd3affbe82c
  • Loading branch information
@klvs
klvs authored and facebook-github-bot-8 committed Jan 18, 2016
1 parent a370641 commit 1303e6d
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 18 deletions.
5 changes: 3 additions & 2 deletions Libraries/Network/RCTNetworking.android.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,16 @@ var generateRequestId = function() {
*/
class RCTNetworking {

static sendRequest(method, url, headers, data, useIncrementalUpdates) {
static sendRequest(method, url, headers, data, useIncrementalUpdates, timeout) {
var requestId = generateRequestId();
RCTNetworkingNative.sendRequest(
method,
url,
requestId,
headers,
data,
useIncrementalUpdates);
useIncrementalUpdates,
timeout);
return requestId;
}

Expand Down
2 changes: 1 addition & 1 deletion Libraries/Network/RCTNetworking.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ - (RCTURLRequestCancellationBlock)buildRequest:(NSDictionary<NSString *, id> *)q
NSMutableURLRequest *request = [NSMutableURLRequest requestWithURL:URL];
request.HTTPMethod = [RCTConvert NSString:RCTNilIfNull(query[@"method"])].uppercaseString ?: @"GET";
request.allHTTPHeaderFields = [RCTConvert NSDictionary:query[@"headers"]];

request.timeoutInterval = [RCTConvert NSTimeInterval:query[@"timeout"]];
NSDictionary<NSString *, id> *data = [RCTConvert NSDictionary:RCTNilIfNull(query[@"data"])];
return [self processDataForHTTPQuery:data callback:^(NSError *error, NSDictionary<NSString *, id> *result) {
if (error) {
Expand Down
6 changes: 3 additions & 3 deletions Libraries/Network/XMLHttpRequest.android.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ function convertHeadersMapToArray(headers: Object): Array<Header> {
}

class XMLHttpRequest extends XMLHttpRequestBase {
sendImpl(method: ?string, url: ?string, headers: Object, data: any): void {
sendImpl(method: ?string, url: ?string, headers: Object, data: any, timeout: number): void {
var body;
if (typeof data === 'string') {
body = {string: data};
Expand All @@ -40,14 +40,14 @@ class XMLHttpRequest extends XMLHttpRequestBase {
} else {
body = data;
}

var useIncrementalUpdates = this.onreadystatechange ? true : false;
var requestId = RCTNetworking.sendRequest(
method,
url,
convertHeadersMapToArray(headers),
body,
useIncrementalUpdates
useIncrementalUpdates,
timeout
);
this.didCreateRequest(requestId);
}
Expand Down
3 changes: 2 additions & 1 deletion Libraries/Network/XMLHttpRequest.ios.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class XMLHttpRequest extends XMLHttpRequestBase {
this.upload = {};
}

sendImpl(method: ?string, url: ?string, headers: Object, data: any): void {
sendImpl(method: ?string, url: ?string, headers: Object, data: any, timeout: number): void {
if (typeof data === 'string') {
data = {string: data};
} else if (data instanceof FormData) {
Expand All @@ -36,6 +36,7 @@ class XMLHttpRequest extends XMLHttpRequestBase {
data,
headers,
incrementalUpdates: this.onreadystatechange ? true : false,
timeout
},
this.didCreateRequest.bind(this)
);
Expand Down
6 changes: 4 additions & 2 deletions Libraries/Network/XMLHttpRequestBase.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class XMLHttpRequestBase {
responseHeaders: ?Object;
responseText: ?string;
status: number;
timeout: number;
responseURL: ?string;

upload: ?{
Expand All @@ -58,6 +59,7 @@ class XMLHttpRequestBase {
this.onreadystatechange = null;
this.onload = null;
this.upload = undefined; /* Upload not supported yet */
this.timeout = 0;

this._reset();
this._method = null;
Expand Down Expand Up @@ -196,7 +198,7 @@ class XMLHttpRequestBase {
this.setReadyState(this.OPENED);
}

sendImpl(method: ?string, url: ?string, headers: Object, data: any): void {
sendImpl(method: ?string, url: ?string, headers: Object, data: any, timeout: number): void {
throw new Error('Subclass must define sendImpl method');
}

Expand All @@ -208,7 +210,7 @@ class XMLHttpRequestBase {
throw new Error('Request has already been sent');
}
this._sent = true;
this.sendImpl(this._method, this._url, this._headers, data);
this.sendImpl(this._method, this._url, this._headers, data, this.timeout);
}

abort(): void {
Expand Down
10 changes: 9 additions & 1 deletion ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
import java.io.InputStream;
import java.io.Reader;

import java.util.concurrent.TimeUnit;

import com.facebook.react.bridge.Arguments;
import com.facebook.react.bridge.GuardedAsyncTask;
import com.facebook.react.bridge.ReactApplicationContext;
Expand Down Expand Up @@ -113,19 +115,25 @@ public void onCatalystInstanceDestroy() {
}

@ReactMethod
/**
* @param timeout value of 0 results in no timeout
*/
public void sendRequest(
String method,
String url,
final int requestId,
ReadableArray headers,
ReadableMap data,
final boolean useIncrementalUpdates) {
final boolean useIncrementalUpdates,
int timeout) {
Request.Builder requestBuilder = new Request.Builder().url(url);

if (requestId != 0) {
requestBuilder.tag(requestId);
}

mClient.setConnectTimeout(timeout, TimeUnit.MILLISECONDS);

This comment has been minimized.

Sign in to view

Copy link
@atticoos

atticoos Jan 24, 2016

Contributor

If I'm not mistaken, the mClient is a singleton instance -- wouldn't this mean that the timeout provided by a single request would apply the rule for the client that manages all requests from that point forward?

Basically this seems that it would get applied at a global level, rather than the request level

Headers requestHeaders = extractHeaders(headers, data);
if (requestHeaders == null) {
onRequestError(requestId, "Unrecognized headers format");
Expand Down
24 changes: 16 additions & 8 deletions ReactAndroid/src/test/java/com/facebook/react/modules/network/NetworkingModuleTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
0,
SimpleArray.of(),
null,
true);
true,
0);

ArgumentCaptor<Request> argumentCaptor = ArgumentCaptor.forClass(Request.class);
verify(httpClient).newCall(argumentCaptor.capture());
Expand Down Expand Up @@ -122,7 +123,8 @@ public void testFailGetWithInvalidHeadersStruct() throws Exception {
0,
SimpleArray.from(invalidHeaders),
null,
true);
true,
0);

verifyErrorEmit(emitter, 0);
}
Expand All @@ -147,7 +149,8 @@ public void testFailPostWithoutContentType() throws Exception {
0,
SimpleArray.of(),
body,
true);
true,
0);

verifyErrorEmit(emitter, 0);
}
Expand Down Expand Up @@ -202,7 +205,8 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
0,
SimpleArray.of(SimpleArray.of("Content-Type", "text/plain")),
body,
true);
true,
0);

ArgumentCaptor<Request> argumentCaptor = ArgumentCaptor.forClass(Request.class);
verify(httpClient).newCall(argumentCaptor.capture());
Expand Down Expand Up @@ -238,7 +242,8 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
0,
SimpleArray.from(headers),
null,
true);
true,
0);
ArgumentCaptor<Request> argumentCaptor = ArgumentCaptor.forClass(Request.class);
verify(httpClient).newCall(argumentCaptor.capture());
Headers requestHeaders = argumentCaptor.getValue().headers();
Expand Down Expand Up @@ -284,7 +289,8 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
0,
new SimpleArray(),
body,
true);
true,
0);

// verify url, method, headers
ArgumentCaptor<Request> argumentCaptor = ArgumentCaptor.forClass(Request.class);
Expand Down Expand Up @@ -341,7 +347,8 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
0,
SimpleArray.from(headers),
body,
true);
true,
0);

// verify url, method, headers
ArgumentCaptor<Request> argumentCaptor = ArgumentCaptor.forClass(Request.class);
Expand Down Expand Up @@ -435,7 +442,8 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
0,
SimpleArray.from(headers),
body,
true);
true,
0);

// verify RequestBodyPart for image
PowerMockito.verifyStatic(times(1));
Expand Down

7 comments on commit 1303e6d

@christopherdro
Copy link
Contributor

Choose a reason for hiding this comment

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

This is breaking for me on master. I can't seem to track down what's going on. Everything appears to be correct. Will try again in the morning.

errormsg

/cc @nicklockwood @foghina

@klvs
Copy link
Contributor Author

@klvs klvs commented on 1303e6d Jan 20, 2016

Choose a reason for hiding this comment

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

Just tested it from a clean react-native init and it seems to be working fine. Unfortunately I'm having issues setting up a test with master right now and will have to wait until tomorrow to trouble shoot.

@nicklockwood
Copy link
Contributor

Choose a reason for hiding this comment

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

@christopherdro you probably have mismatched JS and Java code versions. Seems to happen quite a lot whenever we make changes to native interfaces.

@foghina
Copy link
Contributor

Choose a reason for hiding this comment

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

When running on master you need to build the Android runtime from source. Otherwise, as @nicklockwood is saying, you'll end up with incompatible JS and native modules versions.

@christopherdro
Copy link
Contributor

Choose a reason for hiding this comment

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

@nicklockwood @foghina @klvs
Yea, I figured I was missing something like that. Sorry for the bother & thanks for the tip!

@jslz
Copy link

@jslz jslz commented on 1303e6d Aug 4, 2017

Choose a reason for hiding this comment

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

hi, I guess I want to use this! :-) And there's lots of people asking about support for timeouts in RN it seems. Are there docs about it? I currently use fetch() so I'd want it available there. I don't see it mentioned in docs tho https://facebook.github.io/react-native/docs/network.html

@klvs
Copy link
Contributor Author

@klvs klvs commented on 1303e6d Aug 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@jslz I believe that react-native used whatwg-fetch which doesn't include a timeout option (honestly always wondered why). Since the PR I authored implements the timeout api on XMLHttpRequest, any polyfill that uses it should have a working timeout. Maybe try whatwg-fetch-timout?

Please sign in to comment.