Add support for HTTP 405 and enhance request handling#4
Open
harmon25 wants to merge 6 commits into
Open
Conversation
…h connection limits and timeouts
There was a problem hiding this comment.
Pull request overview
This PR improves AtomVM HTTPD’s protocol compliance and resilience by tightening request parsing/normalization and adding new connection/request management features in the TCP and HTTP layers.
Changes:
- Add HTTP/1.1 request version parsing, lowercase header normalization, and WebSocket upgrade validation.
- Introduce request timeout plumbing and TCP connection tracking/limits.
- Update tests and handler modules to match the new header normalization and request shape.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/httpd.erl |
Adds version parsing, header normalization, WebSocket key validation, keep-alive logic, request timers, and 405 handling. |
src/gen_tcp_server.erl |
Adds max connection tracking/limits, timeout handling, and links accept/loop processes. |
src/httpd_ws_handler.erl |
Makes WS send/2 async, optimizes unmasking, and fixes frame length boundary. |
src/httpd_env_api_handler.erl |
Reduces atom-leak risk by using existing atoms and shifting path/query handling. |
src/httpd_ota_handler.erl |
Updates content-length header access to match normalized request headers. |
src/httpd_handler.erl |
Extends handler request type with version. |
include/httpd.hrl |
Adds HTTP 405 constant. |
test/httpd_integration_test.exs |
Adjusts expectations to lowercase header names in requests/responses. |
test/atomvm_httpd_test.exs |
Updates unit tests for normalized headers and expanded HTTPD internal state tuple. |
Comments suppressed due to low confidence (1)
src/gen_tcp_server.erl:87
max_connectionsis read fromSocketOptions, butSocketOptionsis still passed wholesale intoset_socket_options/2. If the caller provides#{max_connections => N}, this will callsocket:setopt(Socket, max_connections, N)and likely crash during init. Stripmax_connectionsout before applying socket options.
init({BindOptions, SocketOptions, Handler, Args}) ->
Self = self(),
MaxConnections = maps:get(max_connections, SocketOptions, 0),
case socket:open(inet, stream, tcp) of
{ok, Socket} ->
ok = set_socket_options(Socket, SocketOptions),
case socket:bind(Socket, BindOptions) of
…cs, binary types
- gen_tcp_server: strip max_connections from SocketOptions before passing to
set_socket_options/2 to prevent socket:setopt/3 crashing on unknown key
- gen_tcp_server: track connections at accept time (not first-packet) by sending
{new_connection, Socket} immediately after accept; close socket there if
max_connections is exceeded, closing the idle-socket flood gap
- httpd: cancel pre-existing timer ref in start_request_timer/2 before creating
a new one to prevent stale {request_timeout, Socket} messages on keep-alive
- httpd: cancel timer in stop_request_timer/2 (not just remove map entry)
- httpd: cancel timer via stop_request_timer/2 in handle_tcp_closed/2 so the
timer cannot fire after the socket is gone
- httpd: honour handler {close, ...} return unconditionally regardless of
client keep-alive header, preserving the documented httpd_handler contract
- httpd: convert url_decode/2 charlist result to binary in parse_query_param/1
so all query_params() values are binaries as declared in the type spec
- httpd_env_api_handler: drop binary_to_list/1 in DELETE path so segment keys
remain binaries, consistent with the binary keys stored by POST/GET
- test: update handle_request_state assertion to expect a timer ref in
pending_timer_map instead of an empty map
ensure_content_length: normalize response headers to lowercase binary before
writing Content-Length to prevent duplicate headers when callers pass
mixed-case or string keys (e.g. 'Content-Length', <<"Content-Length">>).
Add normalize_headers/1 and normalize_header_key/1 helpers. Update the
WebSocket handshake test to expect lowercase header names in the response.
keep-alive comment: replace misleading 'honour keep-alive if requested' with
an accurate note that {reply,...} always keeps the socket open; defer proper
Connection/version-aware negotiation to a follow-up PR.
Race-free request timeout:
- Change timeout message tag from {request_timeout, Socket} to
{request_timeout, Socket, Tag} where Tag = make_ref() is unique per timer.
- Store {TimerRef, Tag} in pending_timer_map instead of bare TimerRef.
- Remove receive-flush from start_request_timer/2 and stop_request_timer/2;
stale messages are now safely ignored in handle_info/2 via Tag validation.
- Add optional handle_info/2 callback to gen_tcp_server behaviour; the
catch-all handle_info clause forwards unknown messages to the handler when
it exports handle_info/2, expecting {noreply,S} or {close,Socket,S}.
- Remove the old {request_timeout,Socket} clause from gen_tcp_server.
- Implement httpd:handle_info/2: validates Tag against pending_timer_map and
closes the socket only if it matches (genuine timeout), ignoring stale refs.
Configurable request_timeout:
- Add start/5 and start_link/5 arities accepting an Options map before Config,
e.g. start_link(Address, Port, SocketOpts, #{request_timeout => 200}, Config).
- Existing 2/3/4-arity calls still work via backwards-compatible defaults.
- init/1 reads request_timeout from Options (default 30000 ms).
Request timeout tests:
- Add 'request timeout' describe block in httpd_integration_test.exs with its
own setup that starts httpd with a 300ms request_timeout.
- Test: server closes socket when headers are never terminated (no \r\n\r\n).
- Test: server closes socket when declared body is never fully delivered.
Comment on lines
+488
to
+492
| RawVersion = lists:reverse(Tmp), | ||
| Version = case RawVersion of | ||
| [$\r | Clean] -> list_to_binary(Clean); | ||
| _ -> list_to_binary(RawVersion) | ||
| end, |
Comment on lines
89
to
+90
| Self = self(), | ||
| MaxConnections = maps:get(max_connections, SocketOptions, 0), |
Comment on lines
121
to
122
| Self = self(), | ||
| case Handler:init(Args) of |
…able (default 4096)
Issue 1: 'Connection closed mid-transfer' was logged at io:format level on
every client-initiated cancel (tab close, image swap, parallel race). This
fires dozens of times per minute on a camera stream and obscures real errors.
Demoted to ?TRACE — a compile-time no-op at the default log level.
Issue 3: MAX_SEND_CHUNK was hardcoded at 1460 bytes (single TCP MSS), causing
~90 socket:send NIF crossings + receive-after-0 yields per 130 KB JPEG frame.
Replaced with a configurable chunk_size key in the SocketOptions map (default
4096 bytes), reducing NIF crossings by ~65% for typical camera payloads.
- -define(DEFAULT_SEND_CHUNK, 4096) replaces the old 1460 constant
- #state gains send_chunk_size field; init/1 reads + strips chunk_size
from SocketOptions via maps:without/2 (same pattern as max_connections,
prevents badmatch crash if key reached socket:setopt)
- try_send/try_send_iolist/try_send_binary all take MaxChunk as 3rd arg,
threaded from State in handle_tcp_data/3
- README documents chunk_size alongside max_connections in the
application-level socket options section
- Two new integration tests: guards the option-stripping fix and verifies
a 16 KB response body arrives intact through a 512-byte chunk size
Issues 2 (TCP_NODELAY) and 4 (per-connection workers) deferred:
- TCP_NODELAY is not supported by AtomVM's socket NIF (otp_socket.c only
exposes reuseaddr/linger/recvbuf); needs an upstream AtomVM change.
- Per-connection worker refactor is the larger concurrent-send win and
will be addressed in a follow-up.
…oints to it
AGENTS.md absorbs the valuable architecture, handler, routing, and TRACE content
from .github/copilot-instructions.md and adds hard-won repo-specific gotchas:
- AtomVM socket setopt allow-list (no {tcp, nodelay}; crashes on unknown keys)
- app-level keys (max_connections, chunk_size) must be stripped before setopt fold
- socket:send/2 partial-send semantics
- send serialization bottleneck in gen_tcp_server
- TestEchoHandler reply_body semantics (does not echo request body)
- mix format scope (Elixir only), deps no-op, no CI, no priv/ in this repo
- improvements branch vs main
.github/copilot-instructions.md is now a single-paragraph pointer to AGENTS.md.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces significant improvements to the HTTP server and TCP server modules, focusing on connection management, request handling robustness, and protocol compliance. The most notable changes include implementing request timeouts, enforcing a maximum number of TCP connections, improving HTTP header and method handling, and enhancing WebSocket upgrade logic. These updates aim to make the server more resilient and standards-compliant.
Connection and Timeout Management:
pending_timer_mapand arequest_timeoutfield in thestaterecord, along with logic to start and stop timers for each request. This ensures that requests that take too long are properly handled and closed. [1] [2] [3] [4]connectionsandmax_connectionsfields in thestaterecord). The server now refuses new connections when the limit is reached and cleans up connection state when sockets close. [1] [2] [3]HTTP Protocol and Header Handling:
"expect","content-length","connection"). [1] [2]versionfield to thehttp_requestrecord and improved HTTP version parsing, ensuring the server correctly extracts and stores the HTTP version from requests. [1] [2]Request and Response Handling:
is_keep_alive/1function and updating response logic to support persistent connections when requested by the client. [1] [2] [3]405 Method Not Allowedresponse when the method is undefined. [1] [2]WebSocket Upgrade Improvements:
sec-websocket-keyheader, generating the correct reply token, and returning appropriate errors if the header is missing. [1] [2] [3] [4]These changes collectively improve the reliability, security, and standards compliance of the server modules.…h connection limits and timeouts