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.

Sign up for GitHub

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

Jump to bottom

Android range requests #5956

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Android range requests #5956

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8480271
Use unique instances for request headers
steinjak Sep 26, 2022
1bb78ea
Skip in range response according to req header
steinjak Sep 26, 2022
dacbe8e
Merge branch 'ionic-team:main' into android-range-requests
steinjak Jan 4, 2023
0d2ae7c
Renamed response header getter for clarity
steinjak Jan 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 12 additions & 8 deletions android/capacitor/src/main/java/com/getcapacitor/WebViewLocalServer.java
View file
Open in desktop
Expand Up @@ -123,8 +123,8 @@ public String getReasonPhrase() {
return reasonPhrase;
}

public Map<String, String> getResponseHeaders() {
return responseHeaders;
public Map<String, String> buildDefaultResponseHeaders() {
return new HashMap(responseHeaders);
}
}

Expand Down Expand Up @@ -205,14 +205,18 @@ private WebResourceResponse handleLocalRequest(WebResourceRequest request, PathH
if (request.getRequestHeaders().get("Range") != null) {
InputStream responseStream = new LollipopLazyInputStream(handler, request);
String mimeType = getMimeType(path, responseStream);
Map<String, String> tempResponseHeaders = handler.getResponseHeaders();
Map<String, String> tempResponseHeaders = handler.buildDefaultResponseHeaders();
int statusCode = 206;
try {
int totalRange = responseStream.available();
String rangeString = request.getRequestHeaders().get("Range");
String[] parts = rangeString.split("=");
Copy link

Choose a reason for hiding this comment

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

Am just driving by while hunting out my own problem... I wonder if a better way to do this is to make the copy only when it's about to be modified here.

I.e., instead of creating a copy every access, do it when the tempResponseHeaders variable is assigned at line 208:

Map<String, String> tempResponseHeaders = new HashMap(handler.getResponseHeaders());

Other than that though, this code definitely makes good sense to me.

Copy link
Author

Choose a reason for hiding this comment

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

I see what you mean (a getter should not create new objects), but this will re-introduce the problem, as there are other invocations of this method as well - all passing the returned instance into the constructor of WebResourceResponse (which is where the value of tempResponseHeaders also ends up). Note that it's not the headers set on lines 225/226 that cause trouble here, but the Content-Length header, set somewhere else downstream from this code.

We could of course move the cloning to all call points of the getter, but that leaves a trap for the next person coming around and wanting to use this method, probably not knowing about this issue, so we should probably not do it without making the map read-only (through Collections.unmodifiableMap(...), ie. this.responseHeaders = Collections.unmodifiableMap(new LinkedHashMap<String, String>(tempResponseHeaders)) on line 102). The downside to this approach, though, is that it triggers runtime exceptions - and I can't find the automated tests that will trigger these when developing, leaving it to the developer to thoroughly test all cases before submitting a patch...

Maybe it would be better to rename WebViewLocalServer.PathHandler.getResponseHeaders() to WebViewLocalServer.PathHandler.buildDefaultResponseHeaders() to clarify? I don't think I can see a case where the set of response headers need to be shared between responses, only cases where there is a base set of headers to be included in every response.

What do you think, @peitschie ?

String[] streamParts = parts[1].split("-");
String fromRange = streamParts[0];
int bytesToSkip = Integer.parseInt(fromRange);
if (bytesToSkip > 0) {
responseStream.skip(bytesToSkip);
}
int range = totalRange - 1;
if (streamParts.length > 1) {
range = Integer.parseInt(streamParts[1]);
Expand Down Expand Up @@ -241,7 +245,7 @@ private WebResourceResponse handleLocalRequest(WebResourceRequest request, PathH
handler.getEncoding(),
statusCode,
handler.getReasonPhrase(),
handler.getResponseHeaders(),
handler.buildDefaultResponseHeaders(),
responseStream
);
}
Expand All @@ -252,7 +256,7 @@ private WebResourceResponse handleLocalRequest(WebResourceRequest request, PathH
handler.getEncoding(),
handler.getStatusCode(),
handler.getReasonPhrase(),
handler.getResponseHeaders(),
handler.buildDefaultResponseHeaders(),
null
);
}
Expand Down Expand Up @@ -285,7 +289,7 @@ private WebResourceResponse handleLocalRequest(WebResourceRequest request, PathH
handler.getEncoding(),
statusCode,
handler.getReasonPhrase(),
handler.getResponseHeaders(),
handler.buildDefaultResponseHeaders(),
responseStream
);
}
Expand Down Expand Up @@ -316,7 +320,7 @@ private WebResourceResponse handleLocalRequest(WebResourceRequest request, PathH
handler.getEncoding(),
statusCode,
handler.getReasonPhrase(),
handler.getResponseHeaders(),
handler.buildDefaultResponseHeaders(),
responseStream
);
}
Expand Down Expand Up @@ -375,7 +379,7 @@ private WebResourceResponse handleProxyRequest(WebResourceRequest request, PathH
handler.getEncoding(),
handler.getStatusCode(),
handler.getReasonPhrase(),
handler.getResponseHeaders(),
handler.buildDefaultResponseHeaders(),
responseStream
);
}
Expand Down