Skip to content

Commit

Permalink
Fixed fetch abort behaviour for Edge 16+ (improbable-eng#819)
Browse files Browse the repository at this point in the history
  • Loading branch information
MarcusLongmuir authored Dec 23, 2020
1 parent 42df706 commit 7c2c243
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 11 deletions.
3 changes: 2 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ env:
- BROWSER=chrome_52
- BROWSER=chrome_43
- BROWSER=chrome_41
# Edge does not support aborting fetch requests (documented known limitation in project README)
- BROWSER=edge16_win
# Edge < 16 does not support aborting fetch requests (documented known limitation in project README)
- BROWSER=edge15_win DISABLE_ABORT_TESTS=true
- BROWSER=edge14_win DISABLE_ABORT_TESTS=true
- BROWSER=edge13_win DISABLE_ABORT_TESTS=true
Expand Down
2 changes: 1 addition & 1 deletion client/grpc-web/docs/transport.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ It's great that we have more than one choice when it comes to Web Browsers, howe

* gRPC offers four categories of request: unary, server-streaming, client-streaming and bi-directional. Due to limitations of the Browser HTTP primitives (`fetch` and `XMLHttpRequest`), the HTTP/2-based transports provided by `@improbable-eng/grpc-web` can only support unary and server-streaming requests. Attempts to invoke either client-streaming or bi-directional endpoints will result in failure.
* Older versions of Safari (<7) and all versions of Internet Explorer do not provide an efficient way to stream data from a server; this will result in the entire response of a gRPC client-stream being buffered into memory which can cause performance and stability issues for end-users.
* Microsoft Edge does not propagate the cancellation of requests to the server; which can result in memory/process leaks on your server. Track this issue for status.
* Microsoft Edge (<16) does not propagate the cancellation of requests to the server; which can result in memory/process leaks on your server. Track [this issue](https://github.com/improbable-eng/grpc-web/issues/125) for status.

Note that the [Socket-based Transports](#socket-based-transports) alleviate the above issues.

Expand Down
29 changes: 20 additions & 9 deletions client/grpc-web/src/transports/http/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,10 @@ class Fetch implements Transport {
if (this.cancelled) {
// If the request was cancelled before the first pump then cancel it here
this.options.debug && debug("Fetch.pump.cancel at first pump");
this.reader.cancel();
this.reader.cancel().catch(e => {
// This can be ignored. It will likely throw an exception due to the request being aborted
this.options.debug && debug("Fetch.pump.reader.cancel exception", e);
});
return;
}
this.reader.read()
Expand Down Expand Up @@ -67,7 +70,7 @@ class Fetch implements Transport {
headers: this.metadata.toHeaders(),
method: "POST",
body: msgBytes,
signal: this.controller && this.controller.signal
signal: this.controller && this.controller.signal,
}).then((res: Response) => {
this.options.debug && debug("Fetch.response", res);
this.options.onHeaders(new Metadata(res.headers as any), res.status);
Expand Down Expand Up @@ -101,19 +104,27 @@ class Fetch implements Transport {

cancel() {
if (this.cancelled) {
this.options.debug && debug("Fetch.abort.cancel already cancelled");
this.options.debug && debug("Fetch.cancel already cancelled");
return;
}
this.cancelled = true;

if (this.controller) {
this.options.debug && debug("Fetch.cancel.controller.abort");
this.controller.abort();
} else {
this.options.debug && debug("Fetch.cancel.missing abort controller");
}

if (this.reader) {
// If the reader has already been received in the pump then it can be cancelled immediately
this.options.debug && debug("Fetch.abort.cancel");
this.reader.cancel();
this.options.debug && debug("Fetch.cancel.reader.cancel");
this.reader.cancel().catch(e => {
// This can be ignored. It will likely throw an exception due to the request being aborted
this.options.debug && debug("Fetch.cancel.reader.cancel exception", e);
});
} else {
this.options.debug && debug("Fetch.abort.cancel before reader");
}
if (this.controller) {
this.controller.abort();
this.options.debug && debug("Fetch.cancel before reader");
}
}
}
Expand Down
1 change: 1 addition & 0 deletions integration_test/browsers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ function browser(browserName, browserVersion, os, osVersion) {
// Browser versions that should not have any Fetch/XHR differences in functionality to other (tested) versions are
// commented out.
const browsers = {
edge16_win: browser("edge", "16", "Windows", "10"),
edge15_win: browser("edge", "15", "Windows", "10"),
edge14_win: browser("edge", "14", "Windows", "10"),
edge13_win: browser('edge', "13", 'Windows', "10"),
Expand Down

0 comments on commit 7c2c243

Please sign in to comment.