Skip to content

Commit cbaff4f

Browse files
committed
fix(net-http): a request may be sent once, and a status code is 0..999 (#2067, #2069)
#2067 -- HttpClient sent the same HttpRequestMessage as many times as it was asked, and a counting handler received the exact same object twice. That is not merely untidy: the second send reuses a content object the first send may already have consumed, and both sends share one headers map the first handler may have mutated. .NET has refused it since its first version, and this port now raises InvalidOperationException with .NET's own message. The claim is atomic, not merely a flag. .NET uses an interlocked compare-and-exchange precisely so two concurrent sends cannot both win, and std::atomic_flag::test_and_set is the same operation. The test asserts that rather than trusting the type name -- eight threads released at one starting line, two hundred times. The repetition was necessary: the mutation replacing the claim with a non-atomic read-then-set PASSED a single eight-thread round, and only failed once the test was strengthened. That is recorded rather than quietly tidied. The flag belongs to the message, so a second HttpClient cannot resend it either. MarkAsSent() is public because a custom handler invoked directly, without an HttpClient, is a legitimate caller. sizeof(HttpRequestMessage) grows 192 -> 200 under SA-3, pinned by the layout probe. Consumers must rebuild; no source change is needed. #2069 -- HttpResponseMessage accepted any number as a status code: -1, 0, 1000 and 99999 all constructed and IsSuccessStatusCode answered false for each, so a nonsense code was indistinguishable from a real failure. Both the constructor and the setter now validate, with .NET's two checks in .NET's order. The bound is 999, not 599: RFC 9112 makes a status code three digits and .NET accepts every three-digit value rather than only the registered ranges, 0 included. Both facts are asserted, so a later "tightening" cannot land quietly. An out-of-range value never reaches the object. The wire path cannot be affected, because parseStatusLine has required exactly three digits since #2064 -- verified before implementing, not assumed. #2069 needed no member, and neither did #2068. The response layout static_assert is kept and reworded to record that, since the review expected both to need one. +1 net test, two gated pins inverted. Four mutations, all caught. Gate: 17,238 run, 0 failed, 38 executables -- green. Downstream, measured: zero HttpClient sites in either consumer. SR-AUD-314 and SR-AUD-316 -> remediated. This closes the modules/net-http review: #2067, #2068, #2069 and #2071 all landed today. Record: docs/Migration-HttpRequestSendOnceAndStatusCodeDomain.md.
1 parent 2a947c8 commit cbaff4f

8 files changed

Lines changed: 240 additions & 22 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

audit/AUDIT_FINDINGS_INDEX.md

Lines changed: 2 additions & 2 deletions
Large diffs are not rendered by default.
Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration — a request may be sent once, a status code is 0..999 (tickets #2067, #2069)
5+
6+
*2026-08-17.* Two `System.Net.Http` shapes gain the validation .NET has.
7+
8+
* **#2067**`HttpClient` sent the same `HttpRequestMessage` as many times as you asked.
9+
`sizeof(HttpRequestMessage)` grows **192 → 200** under `docs/StandingApprovals.md` SA-3.
10+
**Downstream consumers must be recompiled.**
11+
* **#2069**`HttpResponseMessage` accepted any number as a status code. No layout change.
12+
13+
Both landed under SA-5 for the behaviour.
14+
15+
---
16+
17+
## 1. #2067 — send once
18+
19+
| | Was | Is |
20+
|---|---|---|
21+
| `client.Send(request)` twice with one message | both succeeded; the **same object** reached the handler twice | the second raises `InvalidOperationException` |
22+
| the message afterwards | no observable state | `getWasSentProperty()` is `true` |
23+
| a second `HttpClient` sending the same message | succeeded | raises — the flag belongs to the **message** |
24+
| a different message || unchanged |
25+
26+
The message is .NET's own: *"The request message was already sent. Cannot send the same request
27+
message multiple times."* (`HttpClient.cs:745-751`).
28+
29+
**Why it matters beyond tidiness.** The second send reuses a content object the first send may
30+
already have consumed, and both sends share one headers map that the first handler may have
31+
mutated. .NET has refused this since its first version.
32+
33+
The claim is atomic. .NET uses an interlocked compare-and-exchange
34+
(`HttpRequestMessage.cs:26,173`) precisely so two concurrent sends cannot both win;
35+
`std::atomic_flag::test_and_set` is the same operation, and a test releases eight threads at one
36+
starting line, two hundred times, to assert it rather than trust the type name.
37+
38+
**To migrate:** build a new `HttpRequestMessage` per send. If you were reusing one to retry, the
39+
retry was already sharing consumed content with the original.
40+
41+
## 2. #2069 — the status-code domain
42+
43+
| Status code | Was | Is |
44+
|---|---|---|
45+
| `-1`, `-1000`, `1000`, `99999` | constructed; `IsSuccessStatusCode` false | `ArgumentOutOfRangeException` |
46+
| `0`, `1`, `100`, `599`, `998`, `999` | constructed | **unchanged** |
47+
| `setStatusCodeProperty` with an out-of-range value | accepted | raises, and the old value survives |
48+
49+
The bound is **999, not 599**. RFC 9112 §4 makes a status code three digits, and .NET accepts
50+
every three-digit value rather than only the registered ranges — `0` included. Both checks, in
51+
both places, are transcribed from `HttpResponseMessage.cs:152-159` and `:65-76`.
52+
53+
**The wire path cannot be affected.** `HttpClient::parseStatusLine` has required exactly three
54+
digits since #2064, so a server can never produce a code this rejects.
55+
56+
**To migrate:** nothing, unless you constructed a sentinel response with a code outside 0..999,
57+
which .NET never allowed either.
58+
59+
## 3. Downstream, measured
60+
61+
Neither `cna` nor `mobile-eggbert` references `HttpClient` or `System::Net::Http`**zero sites
62+
in both**. Neither repository was modified. The full-rebuild requirement for #2067's layout
63+
change is recorded here for any future consumer.

modules/net-http/include/System/Net/Http/HttpRequestMessage.hpp

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include "System/Net/Http/detail/HttpFieldValidation.hpp"
99
#include <memory>
1010
#include <string>
11+
#include <atomic>
1112
#include <unordered_map>
1213

1314
namespace System::Net::Http {
@@ -29,6 +30,25 @@ class HttpRequestMessage {
2930
std::shared_ptr<HttpContent> content_;
3031
std::unordered_map<std::string, std::string> headers_;
3132
HttpRequestOptions options_;
33+
/**
34+
* @brief Whether an `HttpClient` has already sent this message.
35+
*
36+
* Ticket #2067 (SR-AUD-314, CCF-019). .NET throws `InvalidOperationException` when one
37+
* `HttpRequestMessage` is sent twice; this port had no such state, and a counting handler
38+
* received the exact same object twice. That is not merely untidy: the second send reuses a
39+
* content object the first send may already have consumed, and both sends share one headers
40+
* map that the first one's handler may have mutated.
41+
*
42+
* .NET's flag is `_sendStatus`, set with an interlocked compare-and-exchange
43+
* (`HttpRequestMessage.cs:26,173`) so two concurrent sends of one message cannot both win.
44+
* `std::atomic_flag`'s `test_and_set` is the same operation, so this port gets the same
45+
* guarantee rather than a racy approximation of it.
46+
*
47+
* Landed under `docs/StandingApprovals.md` SA-3: a private member, no vtable, base-class,
48+
* signature or `noexcept` change, `sizeof` pinned by the layout probe.
49+
*/
50+
std::atomic_flag sent_ = ATOMIC_FLAG_INIT;
51+
3252
public:
3353
/** Constructs an HttpRequestMessage with the default GET method and an empty URI. */
3454
HttpRequestMessage() : method_(HttpMethod::Get()) {}
@@ -94,6 +114,25 @@ class HttpRequestMessage {
94114
return headers_;
95115
}
96116

117+
/**
118+
* @brief Claims this message for one send, returning false if it was already claimed.
119+
*
120+
* Ticket #2067. The counterpart of .NET's `MarkAsSent()`
121+
* (`HttpRequestMessage.cs:173`), which is an interlocked compare-and-exchange for the same
122+
* reason this is a `test_and_set`: two threads sending one message must not both succeed.
123+
*
124+
* `HttpClient::Send` calls it and raises `InvalidOperationException` with .NET's own message
125+
* when it returns false. It is public rather than private because a custom
126+
* `HttpMessageHandler` invoked directly, without an `HttpClient`, is a legitimate caller.
127+
*/
128+
bool MarkAsSent() noexcept { return !sent_.test_and_set(std::memory_order_acq_rel); }
129+
130+
/** @return Whether an `HttpClient` has already sent this message (#2067). */
131+
[[nodiscard]] bool getWasSentProperty() const noexcept {
132+
// test() is const-correct on the flag's value without claiming it.
133+
return sent_.test(std::memory_order_acquire);
134+
}
135+
97136
/** Gets the per-request option collection. */
98137
[[nodiscard]] HttpRequestOptions& getOptionsProperty() noexcept { return options_; }
99138
[[nodiscard]] const HttpRequestOptions& getOptionsProperty() const noexcept { return options_; }

modules/net-http/include/System/Net/Http/HttpResponseMessage.hpp

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
#pragma once
55
#include "System/Net/Http/HttpContent.hpp"
66
#include "System/Net/Http/HttpRequestException.hpp"
7+
#include "System/ArgumentOutOfRangeException.hpp"
78
#include "System/Net/Http/detail/HttpFieldValidation.hpp"
89
#include "System/Net/HttpStatusCode.hpp"
910
#include <memory>
@@ -19,13 +20,55 @@ class HttpResponseMessage {
1920
std::string reasonPhrase_;
2021
std::shared_ptr<HttpContent> content_;
2122
std::unordered_map<std::string, std::string> headers_;
23+
/**
24+
* @brief `0 <= code <= 999`, exactly .NET's bound.
25+
*
26+
* Ticket #2069 (SR-AUD-316's status-code half). Measured before it, `-1`, `0`, `1000` and
27+
* `99999` all constructed, and `getIsSuccessStatusCodeProperty()` answered false for each --
28+
* so a nonsense code was indistinguishable from a real failure. .NET validates in both the
29+
* constructor and the setter, with the same two checks in the same order
30+
* (`HttpResponseMessage.cs:152-159` and `:65-76`):
31+
*
32+
* ```csharp
33+
* ArgumentOutOfRangeException.ThrowIfNegative((int)value, nameof(value));
34+
* ArgumentOutOfRangeException.ThrowIfGreaterThan((int)value, 999, nameof(value));
35+
* ```
36+
*
37+
* The upper bound is **999**, not 599: RFC 9112 §4 makes a status code three digits, and
38+
* .NET accepts every three-digit value rather than only the registered ranges. `0` is
39+
* accepted too, and that is .NET's choice, not an oversight here.
40+
*/
41+
static void throwIfStatusCodeOutOfRange(System::Net::HttpStatusCode value) {
42+
const int code = static_cast<int>(value);
43+
if (code < 0) {
44+
throw System::ArgumentOutOfRangeException("value", "The status code must not be negative.");
45+
}
46+
if (code > 999) {
47+
throw System::ArgumentOutOfRangeException("value", "The status code must not exceed 999.");
48+
}
49+
}
50+
2251
public:
52+
/**
53+
* @brief Constructs a response with @p statusCode.
54+
* @throws System::ArgumentOutOfRangeException if @p statusCode is negative or above 999.
55+
* @see throwIfStatusCodeOutOfRange
56+
*/
2357
explicit HttpResponseMessage(
2458
System::Net::HttpStatusCode statusCode = System::Net::HttpStatusCode::OK)
25-
: statusCode_(statusCode) {}
59+
: statusCode_(statusCode) {
60+
throwIfStatusCodeOutOfRange(statusCode);
61+
}
2662

2763
[[nodiscard]] System::Net::HttpStatusCode getStatusCodeProperty() const { return statusCode_; }
28-
void setStatusCodeProperty(System::Net::HttpStatusCode v) { statusCode_ = v; }
64+
/**
65+
* @brief Sets the status code.
66+
* @throws System::ArgumentOutOfRangeException if @p v is negative or above 999 (#2069).
67+
*/
68+
void setStatusCodeProperty(System::Net::HttpStatusCode v) {
69+
throwIfStatusCodeOutOfRange(v);
70+
statusCode_ = v;
71+
}
2972

3073
[[nodiscard]] const std::string& getReasonPhraseProperty() const { return reasonPhrase_; }
3174

modules/net-http/src/System/Net/Http/HttpClient.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
#include "System/Net/Http/HttpClient.hpp"
66
#include "System/ArgumentOutOfRangeException.hpp"
7+
#include "System/InvalidOperationException.hpp"
78
#include <condition_variable>
89
#include <memory>
910
#include <mutex>
@@ -303,6 +304,16 @@ std::shared_ptr<HttpResponseMessage> HttpClient::Send(
303304
// override a header the caller already set on this specific request) before handing off
304305
// to the handler chain -- matching real .NET's HttpClient applying DefaultRequestHeaders
305306
// ahead of invoking the handler pipeline.
307+
// Ticket #2067. .NET's CheckRequestMessage does exactly this, with exactly this message
308+
// (HttpClient.cs:745-751, SR.net_http_client_request_already_sent). Sending one message
309+
// twice reuses a content object the first send may already have consumed and shares one
310+
// headers map the first handler may have mutated.
311+
if (!request->MarkAsSent()) {
312+
throw System::InvalidOperationException(
313+
"The request message was already sent. Cannot send the same request message "
314+
"multiple times.");
315+
}
316+
306317
for (const auto& [k, v] : defaultHeaders_) {
307318
// Ticket #2068: the "did the caller already set it" test is case-insensitive too. A
308319
// byte-exact find() meant a default named `Accept` was merged on top of a request that

modules/net-http/tests/System/Net/HttpClientTests.cpp

Lines changed: 79 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1888,6 +1888,10 @@ struct HttpRequestMessageLayoutProbe {
18881888
std::shared_ptr<HttpContent> content;
18891889
std::unordered_map<std::string, std::string> headers;
18901890
HttpRequestOptions options;
1891+
// #2067 added exactly one member: the send-once flag, an atomic_flag for the same reason
1892+
// .NET uses an interlocked compare-and-exchange -- two threads sending one message must not
1893+
// both succeed.
1894+
std::atomic_flag sent;
18911895
};
18921896
struct HttpResponseMessageLayoutProbe {
18931897
System::Net::HttpStatusCode statusCode;
@@ -1899,11 +1903,13 @@ struct HttpResponseMessageLayoutProbe {
18991903
} // namespace
19001904

19011905
static_assert(sizeof(HttpRequestMessage) == sizeof(HttpRequestMessageLayoutProbe),
1902-
"#2067's sent-state flag would grow HttpRequestMessage -- SR-AUD-314, "
1903-
"OBJECT LAYOUT CHANGE, NOT APPROVED");
1906+
"HttpRequestMessage's object layout moved. #2067 added exactly one member -- the "
1907+
"send-once flag -- under docs/StandingApprovals.md SA-3. Any further data member "
1908+
"here is a new object-layout change and needs its own approval.");
19041909
static_assert(sizeof(HttpResponseMessage) == sizeof(HttpResponseMessageLayoutProbe),
1905-
"#2068/#2069 must not add state to HttpResponseMessage -- SR-AUD-315/316, "
1906-
"NOT APPROVED");
1910+
"HttpResponseMessage gained state. #2068 (case-insensitive lookup) and #2069 "
1911+
"(status-code validation) both landed WITHOUT adding a member -- neither "
1912+
"needed one, which is why neither needed an approval.");
19071913

19081914
// The comparator of the returned map is PUBLIC SURFACE. #2062's review concluded from that
19091915
// that #2068 "cannot make the lookup case-insensitive without changing this type, which is why
@@ -1930,17 +1936,52 @@ TEST(NetHttpGatedBehaviourPins, LayoutAndOwnershipModelAreStaticallyAsserted) {
19301936
EXPECT_EQ(sizeof(HttpResponseMessage), sizeof(HttpResponseMessageLayoutProbe));
19311937
}
19321938

1933-
// #2067 (SR-AUD-314) -- .NET throws InvalidOperationException when one
1934-
// HttpRequestMessage is sent twice. This port has no sent state.
1935-
TEST(NetHttpGatedBehaviourPins, Pin2067_OneRequestMessageCanStillBeSentTwice) {
1939+
// #2067 (SR-AUD-314) LANDED -- one HttpRequestMessage may be sent once, as in .NET.
1940+
TEST(NetHttpGatedBehaviourPins, Fix2067_OneRequestMessageCannotBeSentTwice) {
1941+
// .NET's CheckRequestMessage, with .NET's own message
1942+
// (HttpClient.cs:745-751, SR.net_http_client_request_already_sent). The second send is not
1943+
// merely untidy: it reuses a content object the first send may already have consumed, and
1944+
// both sends share one headers map the first handler may have mutated.
19361945
auto handler = std::make_shared<RecordingHandler>();
19371946
HttpClient client(handler);
19381947
auto request = std::make_shared<HttpRequestMessage>(HttpMethod::Get(), "http://example.com/");
19391948

1949+
EXPECT_FALSE(request->getWasSentProperty());
19401950
EXPECT_NO_THROW((void)client.Send(request));
1941-
EXPECT_NO_THROW((void)client.Send(request));
1942-
EXPECT_EQ(handler->receivedRequest, request)
1943-
<< "the SAME message object reaches the handler a second time";
1951+
EXPECT_TRUE(request->getWasSentProperty());
1952+
EXPECT_THROW((void)client.Send(request), System::InvalidOperationException);
1953+
1954+
// A DIFFERENT message is unaffected, and so is a second client.
1955+
auto fresh = std::make_shared<HttpRequestMessage>(HttpMethod::Get(), "http://example.com/");
1956+
EXPECT_NO_THROW((void)client.Send(fresh));
1957+
HttpClient other(handler);
1958+
EXPECT_THROW((void)other.Send(request), System::InvalidOperationException)
1959+
<< "the flag belongs to the MESSAGE, not to the client that sent it";
1960+
}
1961+
1962+
TEST(NetHttpGatedBehaviourPins, Fix2067_TheSendOnceClaimIsAtomic) {
1963+
// .NET uses an interlocked compare-and-exchange, so two concurrent sends of one message
1964+
// cannot both win. std::atomic_flag::test_and_set is the same operation; this asserts the
1965+
// guarantee rather than trusting the type name.
1966+
// Repeated, because a lost update is probabilistic: one round of a non-atomic
1967+
// read-then-set usually still yields one winner. Two hundred rounds of eight threads
1968+
// released together does not.
1969+
for (int round = 0; round < 200; ++round) {
1970+
auto request = std::make_shared<HttpRequestMessage>(HttpMethod::Get(), "http://example.com/");
1971+
std::atomic<int> winners{0};
1972+
std::atomic<bool> go{false};
1973+
std::vector<std::thread> threads;
1974+
for (int i = 0; i < 8; ++i) {
1975+
threads.emplace_back([&] {
1976+
while (!go.load(std::memory_order_acquire)) { /* spin to the same starting line */ }
1977+
if (request->MarkAsSent()) ++winners;
1978+
});
1979+
}
1980+
go.store(true, std::memory_order_release);
1981+
for (auto& t : threads) t.join();
1982+
ASSERT_EQ(winners.load(), 1)
1983+
<< "exactly one caller may claim the message (round " << round << ")";
1984+
}
19441985
}
19451986

19461987
// #2068 (SR-AUD-315) LANDED -- field names are compared case-insensitively, as RFC 9110 5.1
@@ -2025,14 +2066,35 @@ TEST(NetHttpGatedBehaviourPins, Fix2068_TheHandlersDefaultFieldsYieldToTheCaller
20252066
<< "and it must be the caller's, not the handler's";
20262067
}
20272068

2028-
// #2069 (SR-AUD-316's status-code half) -- the constructor accepts any number.
2029-
TEST(NetHttpGatedBehaviourPins, Pin2069_ResponseAcceptsAnyStatusNumber) {
2030-
for (int code : {-1, 0, 1000, 99999}) {
2031-
HttpResponseMessage response(static_cast<HttpStatusCode>(code));
2032-
EXPECT_EQ(static_cast<int>(response.getStatusCodeProperty()), code);
2033-
EXPECT_FALSE(response.getIsSuccessStatusCodeProperty());
2034-
EXPECT_THROW(response.EnsureSuccessStatusCode(), HttpRequestException);
2069+
// #2069 (SR-AUD-316's status-code half) LANDED -- the domain is 0..999, exactly .NET's.
2070+
TEST(NetHttpGatedBehaviourPins, Fix2069_TheStatusCodeDomainIsZeroToNineHundredNinetyNine) {
2071+
// HttpResponseMessage.cs:152-159 (constructor) and :65-76 (setter), same two checks in the
2072+
// same order. Before this, -1, 0, 1000 and 99999 all constructed and
2073+
// getIsSuccessStatusCodeProperty answered false for each -- a nonsense code was
2074+
// indistinguishable from a real failure.
2075+
for (int code : {-1, -1000, 1000, 99999}) {
2076+
SCOPED_TRACE(code);
2077+
EXPECT_THROW(HttpResponseMessage(static_cast<HttpStatusCode>(code)),
2078+
System::ArgumentOutOfRangeException);
2079+
HttpResponseMessage response;
2080+
EXPECT_THROW(response.setStatusCodeProperty(static_cast<HttpStatusCode>(code)),
2081+
System::ArgumentOutOfRangeException);
2082+
}
2083+
2084+
// The bound is 999, not 599: RFC 9112 4 makes a status code three digits and .NET accepts
2085+
// every three-digit value rather than only the registered ranges. 0 is accepted too, and
2086+
// that is .NET's choice rather than an oversight here.
2087+
for (int code : {0, 1, 100, 599, 998, 999}) {
2088+
SCOPED_TRACE(code);
2089+
EXPECT_NO_THROW(HttpResponseMessage(static_cast<HttpStatusCode>(code)));
20352090
}
2091+
2092+
// ...and an out-of-range code never reaches the object, so a rejected value cannot be read
2093+
// back later.
2094+
HttpResponseMessage response(HttpStatusCode::OK);
2095+
EXPECT_THROW(response.setStatusCodeProperty(static_cast<HttpStatusCode>(1000)),
2096+
System::ArgumentOutOfRangeException);
2097+
EXPECT_EQ(response.getStatusCodeProperty(), HttpStatusCode::OK);
20362098
}
20372099

20382100
// #2070 (SR-AUD-317) -- the charset is a label; the bytes are always the

plan.sqlite3

0 Bytes
Binary file not shown.

0 commit comments

Comments
 (0)