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

WIP: Extend WebSession to support range requests. #2887

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

WIP: Extend WebSession to support range requests. #2887

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
56 changes: 50 additions & 6 deletions src/sandstorm/web-session.capnp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,12 @@ interface WebSession @0xa50711a14d35a8ce extends(Grain.UiSession) {
matchesNoneOf @7 :List(ETag); # If-None-Match
}

rangeHint @10 :Range;
# Requests that the server returns a range of bytes from the document rather than the entire
# document. If the server accepts the range hint, it will return a `content` response with
# `statusCode` set to `partialContent`. However, the server is also free to ignore the
# `rangeHint`.

additionalHeaders @3 :List(Header);
# Additional headers present in the request. Only whitelisted headers are
# permitted.
Expand Down Expand Up @@ -173,6 +179,7 @@ interface WebSession @0xa50711a14d35a8ce extends(Grain.UiSession) {
mimeType @0 :Text;
content @1 :Data;
encoding @2 :Text; # Content-Encoding header (optional).
range @3 :Range; # Content-Range header (optional).
}

struct PutContent {
Expand All @@ -181,6 +188,7 @@ interface WebSession @0xa50711a14d35a8ce extends(Grain.UiSession) {
mimeType @0 :Text;
Copy link
Collaborator

Choose a reason for hiding this comment

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

A bit of a tangent, but: would it actually break anything to delete PutContent per the comment? the layout is exactly the same so it should be wire compatible. The type IDs are different, but it's hard for me to imagine that anything uses that.

Copy link
Member Author

Choose a reason for hiding this comment

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

It'd be wire-compatible, but not source-compatible. Some apps could have their builds broken. Maybe we don't care that much...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I doubt very many apps actually use this directly, and for the ones that do it's a trivial fix. But probably doesn't make sense to change it as part of this patch.

content @1 :Data;
encoding @2 :Text; # Content-Encoding header (optional).
range @3 :Range; # Content-Range header (optional).
}

struct ETag {
Expand All @@ -190,6 +198,28 @@ interface WebSession @0xa50711a14d35a8ce extends(Grain.UiSession) {
# semantically equivalent
}

struct Range {
# Specifies a range of bytes within a larger blob. Used for `Range` and `Content-Range`
# headers.

start @0 :UInt64;
# Start point of the range, in bytes.

end :union {
# Endpoint of the range, in bytes.

endOfContent @1 :Void;
position @2 :UInt64;
}

fullSize :union {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can tell, this is only valid in responses. Maybe we should remove it, since we only currently use the Range struct in requests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh. It's been a long time since I wrote this, but I wonder if I had intended to include a Range under Response.content but forgot... otherwise I'm not sure how the Content-Range header is supposed to be constructed for the response.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See the comment you wrote below -- looks like the idea was to just insist that the app returns the range that was actually asked for, so the supervisor only needs to check the status code.

Copy link
Member Author

Choose a reason for hiding this comment

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

But how is the overall size communicated? I guess the final Content-Range header would always end up ending in /*?

BTW, what makes you say that fullSize is not valid for requests? It doesn't appear in a GET request (Range header), but a PUT that is uploading a range (Content-Range header) could include it, right? (However, I'm not sure if it's actually worth supporting range PUTs...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I was only thinking about GET requests. I guess I don't feel that strongly about this.

# Size of the overall file (not just this range).

unknown @3 :Void;
bytes @4 :UInt64;
}
}

struct Cookie {
name @0 :Text;
value @1 :Text;
Expand Down Expand Up @@ -243,11 +273,22 @@ interface WebSession @0xa50711a14d35a8ce extends(Grain.UiSession) {
created @1 $httpStatus(id = 201, title = "Created");
accepted @2 $httpStatus(id = 202, title = "Accepted");

noContent @3 $httpStatus(id = 204, title = "No Content");
partialContent @4 $httpStatus(id = 206, title = "Partial Content");
multiStatus @5 $httpStatus(id = 207, title = "Multi-Status");
# Indicates that the server is only returning the range of bytes that the client requested
# via `Context.rangeHint`. This status is only allowed when a `rangeHint` was present.
#
# (In traditional HTTP, the server could return an arbitrary range that has nothing to do
# with the range the client requested. Since this is useless in practice and annoying to
# check for, WebSession only allows the server to return exactly the range the client
# wanted. If an http-bridge app returns a range that doesn't match what the client asked
# for, sandstorm-http-bridge will throw an exception instead.)

# This seems to fit better here than in the 3xx range
# TODO(apibump): Only the three status codes above should actually be used. The status codes
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/three/four/

# below this point actually represent special cases that are represented by using different
# fields of the Response union.

noContent @3 $httpStatus(id = 204, title = "No Content");
multiStatus @5 $httpStatus(id = 207, title = "Multi-Status");
notModified @6 $httpStatus(id = 304, title = "Not Modified");

# Not applicable:
Expand All @@ -273,21 +314,24 @@ interface WebSession @0xa50711a14d35a8ce extends(Grain.UiSession) {
notAcceptable @4 $httpStatus(id = 406, title = "Not Acceptable");
conflict @5 $httpStatus(id = 409, title = "Conflict");
gone @6 $httpStatus(id = 410, title = "Gone");
preconditionFailed @11 $httpStatus(id = 412, title = "Precondition Failed");
requestEntityTooLarge @7 $httpStatus(id = 413, title = "Request Entity Too Large");
requestUriTooLong @8 $httpStatus(id = 414, title = "Request-URI Too Long");
unsupportedMediaType @9 $httpStatus(id = 415, title = "Unsupported Media Type");
rangeNotSatisfiable @13 $httpStatus(id = 416, title = "Range Not Satisfiable");
imATeapot @10 $httpStatus(id = 418, title = "I'm a teapot");
unprocessableEntity @12 $httpStatus(id = 422, title = "Unprocessable Entity");

preconditionFailed @11 $httpStatus(id = 412, title = "Precondition Failed");
# TODO(apibump): This enum value is deprecated. Use the union branch instead.

# Not applicable:
# 401 Unauthorized: We don't do HTTP authentication.
# 402 Payment Required: LOL
# 407 Proxy Authentication Required: Not a proxy.
# 408 Request Timeout: Not possible; the entire request is provided with the call.
# 411 Length Required: Request is framed using Cap'n Proto.
# 412 Precondition Failed: If we implement preconditions, they should be handled
# separately from errors.
# 412 Precondition Failed: Preconditions are handled by a separate union branch.
# TODO(apibump): Oops, 412 is actually defined above. It should be removed.
# 416 Requested Range Not Satisfiable: Ranges not implemented (might be later).
# 417 Expectation Failed: Like 412.
# Others: Not standard.
Expand Down