Skip to content

Commit 21d5180

Browse files
eddyashtoneddyashton
authored
Revert "Match standard HTTP semantics of inclusive end value in Range and Content-Range headers (#7632)" (#7641)
1 parent 9b7828f commit 21d5180

File tree

4 files changed

+16
-47
lines changed

4 files changed

+16
-47
lines changed

‎include/ccf/http_consts.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ namespace ccf
2020
static constexpr auto DIGEST = "digest";
2121
static constexpr auto HOST = "host";
2222
static constexpr auto LOCATION = "location";
23-
static constexpr auto RANGE = "range";
2423
static constexpr auto RETRY_AFTER = "retry-after";
2524
static constexpr auto TRAILER = "trailer";
2625
static constexpr auto WWW_AUTHENTICATE = "www-authenticate";

‎src/node/rpc/file_serving_handlers.h‎

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -160,8 +160,7 @@ namespace ccf::node
160160
size_t range_start = 0;
161161
size_t range_end = total_size;
162162
{
163-
const auto range_header =
164-
ctx.rpc_ctx->get_request_header(ccf::http::headers::RANGE);
163+
const auto range_header = ctx.rpc_ctx->get_request_header("range");
165164
if (range_header.has_value())
166165
{
167166
LOG_TRACE_FMT("Parsing range header {}", range_header.value());
@@ -232,14 +231,10 @@ namespace ccf::node
232231

233232
if (!s_range_end.empty())
234233
{
235-
// Range end in header is inclusive, but we prefer to reason about
236-
// exclusive range end (ie - one past the end)
237-
size_t inclusive_range_end = 0;
238-
239234
// Fully-specified range, like "X-Y"
240235
{
241236
const auto [p, ec] = std::from_chars(
242-
s_range_end.begin(), s_range_end.end(), inclusive_range_end);
237+
s_range_end.begin(), s_range_end.end(), range_end);
243238
if (ec != std::errc())
244239
{
245240
ctx.rpc_ctx->set_error(
@@ -253,8 +248,6 @@ namespace ccf::node
253248
}
254249
}
255250

256-
range_end = inclusive_range_end + 1;
257-
258251
if (range_end > total_size)
259252
{
260253
LOG_DEBUG_FMT(
@@ -340,15 +333,11 @@ namespace ccf::node
340333
ccf::http::headervalues::contenttype::OCTET_STREAM);
341334
ctx.rpc_ctx->set_response_body(std::move(contents));
342335

343-
// Convert back to HTTP-style inclusive range end
344-
const auto inclusive_range_end = range_end - 1;
345-
346336
// Partial Content responses describe the current response in
347337
// Content-Range
348338
ctx.rpc_ctx->set_response_header(
349339
ccf::http::headers::CONTENT_RANGE,
350-
fmt::format(
351-
"bytes {}-{}/{}", range_start, inclusive_range_end, total_size));
340+
fmt::format("bytes {}-{}/{}", range_start, range_end, total_size));
352341
}
353342

354343
static void init_file_serving_handlers(

‎src/snapshots/fetch.h‎

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ namespace snapshots
4343
struct ContentRangeHeader
4444
{
4545
size_t range_start;
46-
size_t inclusive_range_end;
46+
size_t range_end;
4747
size_t total_size;
4848
};
4949

@@ -93,7 +93,7 @@ namespace snapshots
9393

9494
{
9595
const auto [p, ec] = std::from_chars(
96-
range_end.begin(), range_end.end(), parsed_values.inclusive_range_end);
96+
range_end.begin(), range_end.end(), parsed_values.range_end);
9797
if (ec != std::errc())
9898
{
9999
throw std::runtime_error(fmt::format(
@@ -142,7 +142,6 @@ namespace snapshots
142142
constexpr size_t range_size = 4L * 1024 * 1024;
143143
size_t range_start = 0;
144144
size_t range_end = range_size;
145-
size_t inclusive_range_end = range_end - 1;
146145
bool fetched_all = false;
147146

148147
auto process_partial_response =
@@ -161,29 +160,26 @@ namespace snapshots
161160

162161
// The server may give us _less_ than we requested (since they know
163162
// where the file ends), but should never give us more
164-
if (content_range.inclusive_range_end > inclusive_range_end)
163+
if (content_range.range_end > range_end)
165164
{
166165
throw std::runtime_error(fmt::format(
167166
"Unexpected range response. Requested bytes {}-{}, received "
168167
"range ending at {}",
169168
range_start,
170-
inclusive_range_end,
171-
content_range.inclusive_range_end));
169+
range_end,
170+
content_range.range_end));
172171
}
173172

174-
const auto content_range_exclusive_range_end =
175-
content_range.inclusive_range_end + 1;
176-
177173
const auto range_size =
178-
content_range_exclusive_range_end - content_range.range_start;
174+
content_range.range_end - content_range.range_start;
179175
LOG_TRACE_FMT(
180176
"Received {}-byte chunk from {}. Now have {}/{}",
181177
range_size,
182178
request.get_url(),
183-
content_range_exclusive_range_end,
179+
content_range.range_end,
184180
content_range.total_size);
185181

186-
if (content_range_exclusive_range_end == content_range.total_size)
182+
if (content_range.range_end == content_range.total_size)
187183
{
188184
fetched_all = true;
189185
}
@@ -192,7 +188,6 @@ namespace snapshots
192188
// Advance range for next request
193189
range_start = range_end;
194190
range_end = range_start + range_size;
195-
inclusive_range_end = range_end - 1;
196191
}
197192
};
198193

@@ -208,8 +203,7 @@ namespace snapshots
208203

209204
ccf::curl::UniqueSlist headers;
210205
headers.append(
211-
ccf::http::headers::RANGE,
212-
fmt::format("bytes={}-{}", range_start, inclusive_range_end));
206+
"Range", fmt::format("bytes={}-{}", range_start, range_end));
213207

214208
CURLcode curl_response = CURLE_FAILED_INIT;
215209
long status_code = 0;
@@ -287,8 +281,7 @@ namespace snapshots
287281
{
288282
ccf::curl::UniqueSlist headers;
289283
headers.append(
290-
ccf::http::headers::RANGE,
291-
fmt::format("bytes={}-{}", range_start, inclusive_range_end));
284+
"Range", fmt::format("bytes={}-{}", range_start, range_end));
292285

293286
std::unique_ptr<ccf::curl::CurlRequest> snapshot_range_request;
294287
CURLcode curl_response = CURLE_OK;

‎tests/e2e_operations.py‎

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -357,37 +357,25 @@ def do_request(http_verb, *args, **kwargs):
357357
assert r.headers["accept-ranges"] == "bytes", r.headers
358358
total_size = int(r.headers["content-length"])
359359

360-
# Use HTTP-style inclusive range end value
361-
range_max = total_size - 1
362-
363360
a = total_size // 3
364361
b = a * 2
365362
for start, end in [
366363
(0, None),
367-
(0, 0),
368-
(0, range_max),
364+
(0, total_size),
369365
(0, a),
370366
(a, a),
371367
(a, b),
372368
(b, b),
373-
(b, range_max),
369+
(b, total_size),
374370
(b, None),
375-
(range_max, range_max),
376371
]:
377372
range_header_value = f"{start}-{'' if end is None else end}"
378373
r = do_request(
379374
"GET", path, headers={"range": f"bytes={range_header_value}"}
380375
)
381376
assert r.status_code == http.HTTPStatus.PARTIAL_CONTENT.value, r
382-
headers = r.headers
383-
implied_end = range_max if end is None else end
384-
assert int(headers["content-length"]) == implied_end - start + 1
385-
assert (
386-
headers["content-range"]
387-
== f"bytes {start}-{implied_end}/{total_size}"
388-
)
389377

390-
expected = snapshot_data[start : (None if end is None else end + 1)]
378+
expected = snapshot_data[start:end]
391379
actual = r.body.data()
392380
assert (
393381
expected == actual

0 commit comments

Comments
 (0)