Skip to content

Commit 8bd18fe

Browse files
quinnjclaude
andauthored
Fix CodeUnits write ambiguity + 500 fallback after deferred startwrite (#1302, #1303) (#1305)
* Fix CodeUnits write ambiguity + 500 fallback after deferred startwrite (#1302, #1303) #1302: Base defines write(::IO, ::Base.CodeUnits), which is ambiguous with the Stream/_RequestDeadlineWriteIO StridedVector{UInt8} write methods (CodeUnits <: DenseVector{UInt8}). Add disambiguating CodeUnits methods so write(stream, codeunits(s)) dispatches to the bytes path on both the client and server stream sides. #1303: h1 FIXED mode defers the response head at startwrite while flipping response_started, so a handler throw after startwrite skipped the 500 fallback (gated on !response_started) and silently dropped the connection; clients saw "unexpected EOF". Track head_committed -- set only where head bytes actually reach the transport (h1 and h2 paths) -- and answer with a raw 500 when the handler fails with nothing committed. Once the head is on the wire (e.g. chunked), behavior is unchanged: no second head is emitted. Tests: codeunits round-trip through a real handler; deferred-startwrite throw now yields 500; chunked control asserts no spurious 500 after a committed head. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(server): retry raw probes that get no first byte on Windows The raw HTTP/1 probe helper fails with "timed out waiting for first response byte" when the known Windows IOCP wake strand (JuliaServices/Reseau.jl#107) leaves the probe's parked reader unwoken — the same canary test has now struck four unrelated PRs. Until the Reseau fix lands, re-dial fresh and retry (3 attempts, Windows only; other platforms stay strict) with a loud warning so the strand remains visible in logs. Safe for all current callers: none assert on server-side invocation counts. The first-byte timeout is now a typed exception so the retry can unwrap it through the nested watchdog wrappers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(client): cover codeunits request bodies through the deadline write IO Covers the Stream{true} and _RequestDeadlineWriteIO CodeUnits disambiguation methods added for #1302. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 94c71fe commit 8bd18fe

6 files changed

Lines changed: 164 additions & 17 deletions

File tree

src/http_server.jl

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,11 @@ mutable struct Stream{ISCLIENT,Req<:Request} <: IO
177177
@atomic write_closed::Bool
178178
@atomic read_closed::Bool
179179
@atomic response_started::Bool
180+
# `response_started` flips at `startwrite` even when the response head is
181+
# deferred (h1 FIXED mode); `head_committed` flips only when head bytes have
182+
# actually been written to the transport, so error paths can tell whether a
183+
# raw error response is still possible (#1303).
184+
@atomic head_committed::Bool
180185
@atomic continue_sent::Bool
181186
ignore_writes::Bool
182187
write_mode::_ServerStreamWriteMode.T
@@ -242,6 +247,7 @@ function Stream(server::Server, tracked::_ServerConn, request::Req) where {Req<:
242247
false,
243248
false,
244249
false,
250+
false,
245251
_ServerStreamWriteMode.UNDECIDED,
246252
Int64(0),
247253
)
@@ -290,6 +296,7 @@ function Stream(request::Req) where {Req<:Request}
290296
false,
291297
false,
292298
false,
299+
false,
293300
_ServerStreamWriteMode.UNDECIDED,
294301
Int64(0),
295302
)
@@ -346,6 +353,7 @@ function Stream(
346353
false,
347354
false,
348355
false,
356+
false,
349357
_ServerStreamWriteMode.UNDECIDED,
350358
Int64(0),
351359
)
@@ -1107,6 +1115,13 @@ function _serve_h1_conn!(server::Server, tracked::_ServerConn, reader_source)::N
11071115
startwrite(stream)
11081116
closewrite(stream)
11091117
end
1118+
elseif !(@atomic :acquire stream.head_committed)
1119+
# startwrite ran but the response head was deferred (h1
1120+
# FIXED mode) and never reached the wire: answer with a
1121+
# raw error response instead of silently dropping the
1122+
# connection (#1303).
1123+
_try_write_server_error!(tracked.conn, request, status === nothing ? 500 : status::Int)
1124+
return nothing
11101125
end
11111126
@try_ignore begin
11121127
stream.response.close = true

src/http_server_streams.jl

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ function _write_server_stream_head!(stream::Stream)::Nothing
8686
end_stream,
8787
_server_write_deadline_ns(stream.server::Server),
8888
)
89+
@atomic :release stream.head_committed = true
8990
@atomic :release stream.response_started = true
9091
return nothing
9192
end
@@ -109,6 +110,7 @@ function _write_server_stream_head!(stream::Stream)::Nothing
109110
_write_headers!(io, headers)
110111
write(io, "\r\n")
111112
_write_server_stream_bytes!(stream, take!(io), false)
113+
@atomic :release stream.head_committed = true
112114
@atomic :release stream.response_started = true
113115
return nothing
114116
end

src/http_stream.jl

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ function Stream(
8989
false,
9090
false,
9191
false,
92+
false,
9293
_ServerStreamWriteMode.UNDECIDED,
9394
Int64(0),
9495
)
@@ -246,6 +247,16 @@ function write(stream::Stream{true}, data::StridedVector{UInt8})::Int
246247
end
247248
write(stream::Stream{false}, data::StridedVector{UInt8}) = _server_write(stream, data)
248249

250+
# Base defines write(::IO, ::Base.CodeUnits); without these methods, writing
251+
# codeunits to a Stream is ambiguous (CodeUnits <: DenseVector{UInt8}
252+
# intersects the StridedVector methods above) (#1302).
253+
function write(stream::Stream{true}, data::Base.CodeUnits{UInt8})::Int
254+
(@atomic :acquire stream.started) && throw(ArgumentError("cannot write request body after response reading has started"))
255+
(@atomic :acquire stream.write_closed) && throw(ArgumentError("request body writes are closed"))
256+
return write(stream.request_buffer, data)
257+
end
258+
write(stream::Stream{false}, data::Base.CodeUnits{UInt8}) = _server_write(stream, data)
259+
249260
function write(stream::Stream{true}, data::AbstractVector{UInt8})::Int
250261
(@atomic :acquire stream.started) && throw(ArgumentError("cannot write request body after response reading has started"))
251262
(@atomic :acquire stream.write_closed) && throw(ArgumentError("request body writes are closed"))

src/http_transport.jl

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,8 @@ end
7878
Base.write(io::_RequestDeadlineWriteIO, data::Vector{UInt8})::Int = _request_deadline_write_bytes!(io, data)
7979
Base.write(io::_RequestDeadlineWriteIO, data::StridedVector{UInt8})::Int = _request_deadline_write_bytes!(io, data)
8080
Base.write(io::_RequestDeadlineWriteIO, data::AbstractVector{UInt8})::Int = _request_deadline_write_bytes!(io, data)
81+
# disambiguate vs Base's write(::IO, ::Base.CodeUnits) (#1302)
82+
Base.write(io::_RequestDeadlineWriteIO, data::Base.CodeUnits{UInt8})::Int = _request_deadline_write_bytes!(io, data)
8183

8284
# Hook unsafe_write instead of write(::IO, ::Union{String, SubString{String}}):
8385
# Base's generic string write funnels through unsafe_write, so strings still

test/http_client_tests.jl

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2385,3 +2385,25 @@ end
23852385
HT.forceclose(server)
23862386
end
23872387
end
2388+
2389+
@testset "HTTP client codeunits bodies dispatch without Base ambiguity (#1302)" begin
2390+
server = HTTP.serve!("127.0.0.1", 0) do req
2391+
body = req.body === nothing ? UInt8[] : req.body
2392+
return HT.Response(200, String(copy(body)))
2393+
end
2394+
try
2395+
url = "http://127.0.0.1:$(HTTP.port(server))/"
2396+
# codeunits request body with a write deadline: routes through the
2397+
# deadline write IO, which was ambiguous with Base's CodeUnits write
2398+
resp = HT.post(url; body = codeunits("cu-body"), write_idle_timeout = 5)
2399+
@test resp.status == 200
2400+
@test String(resp.body) == "cu-body"
2401+
# client stream write of codeunits (Stream{true} disambiguation)
2402+
resp_open = HT.open("POST", url) do io
2403+
write(io, codeunits("cu-open"))
2404+
end
2405+
@test resp_open.status == 200
2406+
finally
2407+
close(server)
2408+
end
2409+
end

test/http_server_http1_tests.jl

Lines changed: 112 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -52,32 +52,59 @@ end
5252
# indefinitely. Wrap the whole exchange in `_run_with_timeout`, a task-level
5353
# watchdog that does not depend on socket deadlines, so a stuck read fails the
5454
# test in seconds instead of hanging the job until CI's wall-clock limit.
55+
# Raised when the raw probe saw no response bytes at all within its budget.
56+
# On Windows this is the known IOCP wake strand (JuliaServices/Reseau.jl#107):
57+
# the parked reader is intermittently never woken even though bytes arrive.
58+
struct _FirstByteTimeout <: Exception end
59+
60+
# `_read_until_quiet` runs under its own `_run_with_timeout` watchdog, so the
61+
# timeout surfaces here wrapped in (possibly nested) TaskFailedException.
62+
function _is_first_byte_timeout(err)::Bool
63+
err isa _FirstByteTimeout && return true
64+
err isa TaskFailedException || return false
65+
inner = err.task.exception
66+
return inner !== nothing && _is_first_byte_timeout(inner)
67+
end
68+
5569
function _raw_http_request(
5670
port::Integer,
5771
request::AbstractString;
5872
settle_s::Float64 = 0.5,
5973
close_write::Bool = true,
6074
wait_for_first_byte::Bool = true,
75+
# Re-dial fresh and retry when no first byte arrives. Windows-only by
76+
# default so other platforms stay strict; safe for these probes because no
77+
# caller asserts on server-side invocation counts.
78+
attempts::Int = Sys.iswindows() ? 3 : 1,
6179
)::String
62-
return _run_with_timeout(; timeout_s = max(8.0, settle_s + 4.0), label = "raw http request (port $(port))") do
63-
sock = ND.connect("tcp", "127.0.0.1:$(Int(port))")
64-
try
65-
write(sock, Vector{UInt8}(codeunits(String(request))))
66-
if close_write
67-
HT.@try_ignore begin
68-
NC.closewrite(sock)
80+
return _run_with_timeout(; timeout_s = max(8.0, settle_s + 4.0) * attempts, label = "raw http request (port $(port))") do
81+
for attempt in 1:attempts
82+
sock = ND.connect("tcp", "127.0.0.1:$(Int(port))")
83+
try
84+
write(sock, Vector{UInt8}(codeunits(String(request))))
85+
if close_write
86+
HT.@try_ignore begin
87+
NC.closewrite(sock)
88+
end
6989
end
90+
# master hardening kept: longer first-byte budget on Windows,
91+
# combined with the re-dial retry below
92+
first_byte_timeout_s = max(Sys.iswindows() ? 5.0 : 2.0, settle_s + 1.0)
93+
return _read_until_quiet(
94+
sock;
95+
timeout_s = first_byte_timeout_s,
96+
quiet_timeout_s = min(0.25, max(0.05, settle_s)),
97+
wait_for_first_byte = wait_for_first_byte,
98+
)
99+
catch err
100+
_is_first_byte_timeout(err) || rethrow(err)
101+
attempt < attempts || error("timed out waiting for first response byte ($(attempts) attempts)")
102+
@warn "raw probe got no first byte; re-dialing — known Windows IOCP wake strand (Reseau#107)" attempt port
103+
finally
104+
NC.close(sock)
70105
end
71-
first_byte_timeout_s = max(Sys.iswindows() ? 5.0 : 2.0, settle_s + 1.0)
72-
return _read_until_quiet(
73-
sock;
74-
timeout_s = first_byte_timeout_s,
75-
quiet_timeout_s = min(0.25, max(0.05, settle_s)),
76-
wait_for_first_byte = wait_for_first_byte,
77-
)
78-
finally
79-
NC.close(sock)
80106
end
107+
error("unreachable: raw probe retry loop exited")
81108
end
82109
end
83110

@@ -166,7 +193,7 @@ function _read_until_quiet(
166193
end
167194
end
168195
if wait_for_first_byte && !saw_bytes
169-
error("timed out waiting for first response byte")
196+
throw(_FirstByteTimeout())
170197
end
171198
return String(out)
172199
end
@@ -949,6 +976,74 @@ end
949976
end
950977
end
951978

979+
@testset "HTTP stream writes accept codeunits without Base ambiguity (#1302)" begin
980+
# Base defines write(::IO, ::Base.CodeUnits); the Stream StridedVector
981+
# methods used to be ambiguous with it.
982+
server = HT.listen!("127.0.0.1", 0; listenany = true) do stream
983+
_ = HT.startread(stream)
984+
HT.setstatus(stream, 200)
985+
HT.setheader(stream, "Content-Length" => "2")
986+
HT.startwrite(stream)
987+
write(stream, codeunits("ok"))
988+
return nothing
989+
end
990+
address = HT.server_addr(server)
991+
try
992+
resp = HT.get("http://$(address)/")
993+
@test resp.status == 200
994+
@test String(resp.body) == "ok"
995+
finally
996+
_run_with_timeout(() -> HT.forceclose(server); label = "server forceclose")
997+
_run_with_timeout(() -> wait(server); label = "server task completion")
998+
end
999+
end
1000+
1001+
@testset "HTTP stream handler error after deferred startwrite returns 500 (#1303)" begin
1002+
# h1 FIXED mode defers the response head at startwrite; a handler throw
1003+
# before anything reaches the wire must produce a raw 500, not a silent
1004+
# connection drop the client sees as "unexpected EOF".
1005+
server = HT.listen!("127.0.0.1", 0; listenany = true) do stream
1006+
_ = HT.startread(stream)
1007+
HT.setstatus(stream, 200)
1008+
HT.setheader(stream, "Content-Length" => "2")
1009+
HT.startwrite(stream) # FIXED: response_started, head NOT committed
1010+
error("boom")
1011+
end
1012+
address = HT.server_addr(server)
1013+
try
1014+
resp = HT.request("GET", "http://$(address)/"; status_exception = false, retry = false)
1015+
@test resp.status == 500
1016+
finally
1017+
_run_with_timeout(() -> HT.forceclose(server); label = "server forceclose")
1018+
_run_with_timeout(() -> wait(server); label = "server task completion")
1019+
end
1020+
1021+
# control: once the head is committed (chunked writes it at startwrite), a
1022+
# handler throw must NOT emit a spurious 500 after the real head
1023+
server2 = HT.listen!("127.0.0.1", 0; listenany = true) do stream
1024+
_ = HT.startread(stream)
1025+
HT.setstatus(stream, 200)
1026+
HT.setheader(stream, "Transfer-Encoding" => "chunked")
1027+
HT.startwrite(stream) # chunked: head committed to the wire here
1028+
error("boom")
1029+
end
1030+
address2 = HT.server_addr(server2)
1031+
try
1032+
outcome = try
1033+
resp2 = HT.request("GET", "http://$(address2)/"; status_exception = false, retry = false)
1034+
resp2.status
1035+
catch err
1036+
err
1037+
end
1038+
# truncated 200 or a client-side error are both acceptable; a 500 means
1039+
# the server wrote a second head after the committed one
1040+
@test outcome != 500
1041+
finally
1042+
_run_with_timeout(() -> HT.forceclose(server2); label = "server forceclose")
1043+
_run_with_timeout(() -> wait(server2); label = "server task completion")
1044+
end
1045+
end
1046+
9521047
@testset "HTTP stream byte I/O supports generic AbstractStrings and Chars" begin
9531048
# Strings are written via unsafe_write; generic AbstractStrings and Chars
9541049
# fall back to Base's per-byte path, which requires write(io, ::UInt8).

0 commit comments

Comments
 (0)