Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed fetch abort behaviour for Edge 16+ #819

Merged
merged 1 commit into from
Dec 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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