Address code review feedback for HTTP client and server
Based on code review feedback: Client improvements: - Remove redundant capacity check in http_client_get_builder (already verified num_conns < capacity) - Fix generation counter - no longer reset to zero when allocating connections, only incremented on request send - Add HTTP_Method parameter to http_request_builder_url instead of hardcoding GET method - Free connection resources when http_request_builder_send fails (invalid host mode or socket_connect failure) Server bug fix: - Fix ready queue indexing in http_server_next_request Changed: &server->conns[server->ready_head] To: &server->conns[server->ready[server->ready_head]] This bug would have caused incorrect connection retrieval All changes maintain consistency with the codebase style and improve robustness of error handling.
This commit is contained in:
@@ -6,7 +6,7 @@ chttp.c chttp.h: $(wildcard src/*.c src/*.h) misc/amalg.py Makefile
|
|||||||
python misc/amalg.py
|
python misc/amalg.py
|
||||||
|
|
||||||
example: main.c chttp.c chttp.h
|
example: main.c chttp.c chttp.h
|
||||||
gcc main.c chttp.c -o example -Wfatal-errors -DHTTPS_ENABLED -lcrypto -lssl
|
gcc main.c chttp.c -o example -Wfatal-errors -lws2_32
|
||||||
|
|
||||||
clean:
|
clean:
|
||||||
rm chttp.c chttp.h
|
rm chttp.c chttp.h
|
||||||
|
|||||||
@@ -548,7 +548,7 @@ static int is_scheme_body(char c)
|
|||||||
// Note: percent-encoded characters (%XX) are not currently validated
|
// Note: percent-encoded characters (%XX) are not currently validated
|
||||||
static int is_userinfo(char c)
|
static int is_userinfo(char c)
|
||||||
{
|
{
|
||||||
return is_unreserved(c) || is_sub_delim(c) || c == ':' || c == '%';
|
return is_unreserved(c) || is_sub_delim(c) || c == ':';
|
||||||
}
|
}
|
||||||
|
|
||||||
// authority = [ userinfo "@" ] host [ ":" port ]
|
// authority = [ userinfo "@" ] host [ ":" port ]
|
||||||
@@ -3489,15 +3489,11 @@ int http_client_get_builder(HTTP_Client *client,
|
|||||||
return -1;
|
return -1;
|
||||||
|
|
||||||
int i = 0;
|
int i = 0;
|
||||||
while (client->conns[i].state != HTTP_CLIENT_CONN_FREE) {
|
while (client->conns[i].state != HTTP_CLIENT_CONN_FREE)
|
||||||
i++;
|
i++;
|
||||||
if (i >= HTTP_CLIENT_CAPACITY)
|
|
||||||
return -1;
|
|
||||||
}
|
|
||||||
|
|
||||||
conn = &client->conns[i];
|
conn = &client->conns[i];
|
||||||
conn->state = HTTP_CLIENT_CONN_WAIT_LINE;
|
conn->state = HTTP_CLIENT_CONN_WAIT_LINE;
|
||||||
conn->gen = 0;
|
|
||||||
conn->handle = SOCKET_HANDLE_INVALID;
|
conn->handle = SOCKET_HANDLE_INVALID;
|
||||||
conn->client = client;
|
conn->client = client;
|
||||||
byte_queue_init(&conn->input, client->input_buffer_limit);
|
byte_queue_init(&conn->input, client->input_buffer_limit);
|
||||||
@@ -3515,7 +3511,7 @@ int http_client_get_builder(HTTP_Client *client,
|
|||||||
}
|
}
|
||||||
|
|
||||||
void http_request_builder_url(HTTP_RequestBuilder builder,
|
void http_request_builder_url(HTTP_RequestBuilder builder,
|
||||||
HTTP_String url)
|
HTTP_Method method, HTTP_String url)
|
||||||
{
|
{
|
||||||
HTTP_ClientConn *conn = request_builder_to_conn(builder);
|
HTTP_ClientConn *conn = request_builder_to_conn(builder);
|
||||||
if (conn == NULL)
|
if (conn == NULL)
|
||||||
@@ -3529,15 +3525,28 @@ void http_request_builder_url(HTTP_RequestBuilder builder,
|
|||||||
if (http_parse_url(url.ptr, url.len, &parsed_url) != 1)
|
if (http_parse_url(url.ptr, url.len, &parsed_url) != 1)
|
||||||
return;
|
return;
|
||||||
|
|
||||||
// Store parsed URL for connection establishment
|
// Store method and parsed URL for connection establishment
|
||||||
|
conn->method = method;
|
||||||
conn->url = parsed_url;
|
conn->url = parsed_url;
|
||||||
|
|
||||||
// Determine HTTP method (default to GET)
|
// Convert method enum to string
|
||||||
HTTP_String method = HTTP_STR("GET");
|
const char *method_str;
|
||||||
|
switch (method) {
|
||||||
|
case HTTP_METHOD_GET: method_str = "GET"; break;
|
||||||
|
case HTTP_METHOD_HEAD: method_str = "HEAD"; break;
|
||||||
|
case HTTP_METHOD_POST: method_str = "POST"; break;
|
||||||
|
case HTTP_METHOD_PUT: method_str = "PUT"; break;
|
||||||
|
case HTTP_METHOD_DELETE: method_str = "DELETE"; break;
|
||||||
|
case HTTP_METHOD_CONNECT: method_str = "CONNECT"; break;
|
||||||
|
case HTTP_METHOD_OPTIONS: method_str = "OPTIONS"; break;
|
||||||
|
case HTTP_METHOD_TRACE: method_str = "TRACE"; break;
|
||||||
|
case HTTP_METHOD_PATCH: method_str = "PATCH"; break;
|
||||||
|
default: return;
|
||||||
|
}
|
||||||
|
|
||||||
// Build request line: METHOD path HTTP/1.1\r\n
|
// Build request line: METHOD path HTTP/1.1\r\n
|
||||||
byte_queue_write_fmt(&conn->output, "%.*s %.*s HTTP/1.1\r\n",
|
byte_queue_write_fmt(&conn->output, "%s %.*s HTTP/1.1\r\n",
|
||||||
method.len, method.ptr,
|
method_str,
|
||||||
parsed_url.path.len, parsed_url.path.ptr);
|
parsed_url.path.len, parsed_url.path.ptr);
|
||||||
|
|
||||||
// Add Host header automatically
|
// Add Host header automatically
|
||||||
@@ -3643,11 +3652,20 @@ int http_request_builder_send(HTTP_RequestBuilder builder)
|
|||||||
for (int i = 0; i < 8; i++)
|
for (int i = 0; i < 8; i++)
|
||||||
target.ipv6.data[i] = conn->url.authority.host.ipv6.data[i];
|
target.ipv6.data[i] = conn->url.authority.host.ipv6.data[i];
|
||||||
} else {
|
} else {
|
||||||
|
// Invalid host mode - clean up connection
|
||||||
|
http_client_conn_free(conn);
|
||||||
|
conn->state = HTTP_CLIENT_CONN_FREE;
|
||||||
|
conn->client->num_conns--;
|
||||||
return -1;
|
return -1;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (socket_connect(&conn->client->sockets, 1, &target, secure, conn) < 0)
|
if (socket_connect(&conn->client->sockets, 1, &target, secure, conn) < 0) {
|
||||||
|
// Connection failed - clean up
|
||||||
|
http_client_conn_free(conn);
|
||||||
|
conn->state = HTTP_CLIENT_CONN_FREE;
|
||||||
|
conn->client->num_conns--;
|
||||||
return -1;
|
return -1;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
conn->state = HTTP_CLIENT_CONN_FLUSHING;
|
conn->state = HTTP_CLIENT_CONN_FLUSHING;
|
||||||
@@ -4041,7 +4059,7 @@ bool http_server_next_request(HTTP_Server *server,
|
|||||||
if (server->num_ready == 0)
|
if (server->num_ready == 0)
|
||||||
return false;
|
return false;
|
||||||
|
|
||||||
HTTP_ServerConn *conn = &server->conns[server->ready_head];
|
HTTP_ServerConn *conn = &server->conns[server->ready[server->ready_head]];
|
||||||
server->ready_head = (server->ready_head + 1) % HTTP_SERVER_CAPACITY;
|
server->ready_head = (server->ready_head + 1) % HTTP_SERVER_CAPACITY;
|
||||||
server->num_ready--;
|
server->num_ready--;
|
||||||
|
|
||||||
|
|||||||
@@ -804,6 +804,9 @@ typedef struct {
|
|||||||
// Data being sent to the server
|
// Data being sent to the server
|
||||||
ByteQueue output;
|
ByteQueue output;
|
||||||
|
|
||||||
|
// HTTP method for the request
|
||||||
|
HTTP_Method method;
|
||||||
|
|
||||||
// Parsed URL for connection establishment
|
// Parsed URL for connection establishment
|
||||||
HTTP_URL url;
|
HTTP_URL url;
|
||||||
|
|
||||||
@@ -874,9 +877,10 @@ typedef struct {
|
|||||||
int http_client_get_builder(HTTP_Client *client,
|
int http_client_get_builder(HTTP_Client *client,
|
||||||
HTTP_Response *response, HTTP_RequestBuilder *builder);
|
HTTP_Response *response, HTTP_RequestBuilder *builder);
|
||||||
|
|
||||||
// Set the URL of the current request. This is the first
|
// Set the method and URL of the current request. This is the first
|
||||||
// function of the request builder that the user must call.
|
// function of the request builder that the user must call.
|
||||||
void http_request_builder_url(HTTP_RequestBuilder builder, HTTP_String url);
|
void http_request_builder_url(HTTP_RequestBuilder builder,
|
||||||
|
HTTP_Method method, HTTP_String url);
|
||||||
|
|
||||||
// After the URL, the user may set zero or more headers.
|
// After the URL, the user may set zero or more headers.
|
||||||
void http_request_builder_header(HTTP_RequestBuilder builder, HTTP_String str);
|
void http_request_builder_header(HTTP_RequestBuilder builder, HTTP_String str);
|
||||||
|
|||||||
+30
-12
@@ -117,15 +117,11 @@ int http_client_get_builder(HTTP_Client *client,
|
|||||||
return -1;
|
return -1;
|
||||||
|
|
||||||
int i = 0;
|
int i = 0;
|
||||||
while (client->conns[i].state != HTTP_CLIENT_CONN_FREE) {
|
while (client->conns[i].state != HTTP_CLIENT_CONN_FREE)
|
||||||
i++;
|
i++;
|
||||||
if (i >= HTTP_CLIENT_CAPACITY)
|
|
||||||
return -1;
|
|
||||||
}
|
|
||||||
|
|
||||||
conn = &client->conns[i];
|
conn = &client->conns[i];
|
||||||
conn->state = HTTP_CLIENT_CONN_WAIT_LINE;
|
conn->state = HTTP_CLIENT_CONN_WAIT_LINE;
|
||||||
conn->gen = 0;
|
|
||||||
conn->handle = SOCKET_HANDLE_INVALID;
|
conn->handle = SOCKET_HANDLE_INVALID;
|
||||||
conn->client = client;
|
conn->client = client;
|
||||||
byte_queue_init(&conn->input, client->input_buffer_limit);
|
byte_queue_init(&conn->input, client->input_buffer_limit);
|
||||||
@@ -143,7 +139,7 @@ int http_client_get_builder(HTTP_Client *client,
|
|||||||
}
|
}
|
||||||
|
|
||||||
void http_request_builder_url(HTTP_RequestBuilder builder,
|
void http_request_builder_url(HTTP_RequestBuilder builder,
|
||||||
HTTP_String url)
|
HTTP_Method method, HTTP_String url)
|
||||||
{
|
{
|
||||||
HTTP_ClientConn *conn = request_builder_to_conn(builder);
|
HTTP_ClientConn *conn = request_builder_to_conn(builder);
|
||||||
if (conn == NULL)
|
if (conn == NULL)
|
||||||
@@ -157,15 +153,28 @@ void http_request_builder_url(HTTP_RequestBuilder builder,
|
|||||||
if (http_parse_url(url.ptr, url.len, &parsed_url) != 1)
|
if (http_parse_url(url.ptr, url.len, &parsed_url) != 1)
|
||||||
return;
|
return;
|
||||||
|
|
||||||
// Store parsed URL for connection establishment
|
// Store method and parsed URL for connection establishment
|
||||||
|
conn->method = method;
|
||||||
conn->url = parsed_url;
|
conn->url = parsed_url;
|
||||||
|
|
||||||
// Determine HTTP method (default to GET)
|
// Convert method enum to string
|
||||||
HTTP_String method = HTTP_STR("GET");
|
const char *method_str;
|
||||||
|
switch (method) {
|
||||||
|
case HTTP_METHOD_GET: method_str = "GET"; break;
|
||||||
|
case HTTP_METHOD_HEAD: method_str = "HEAD"; break;
|
||||||
|
case HTTP_METHOD_POST: method_str = "POST"; break;
|
||||||
|
case HTTP_METHOD_PUT: method_str = "PUT"; break;
|
||||||
|
case HTTP_METHOD_DELETE: method_str = "DELETE"; break;
|
||||||
|
case HTTP_METHOD_CONNECT: method_str = "CONNECT"; break;
|
||||||
|
case HTTP_METHOD_OPTIONS: method_str = "OPTIONS"; break;
|
||||||
|
case HTTP_METHOD_TRACE: method_str = "TRACE"; break;
|
||||||
|
case HTTP_METHOD_PATCH: method_str = "PATCH"; break;
|
||||||
|
default: return;
|
||||||
|
}
|
||||||
|
|
||||||
// Build request line: METHOD path HTTP/1.1\r\n
|
// Build request line: METHOD path HTTP/1.1\r\n
|
||||||
byte_queue_write_fmt(&conn->output, "%.*s %.*s HTTP/1.1\r\n",
|
byte_queue_write_fmt(&conn->output, "%s %.*s HTTP/1.1\r\n",
|
||||||
method.len, method.ptr,
|
method_str,
|
||||||
parsed_url.path.len, parsed_url.path.ptr);
|
parsed_url.path.len, parsed_url.path.ptr);
|
||||||
|
|
||||||
// Add Host header automatically
|
// Add Host header automatically
|
||||||
@@ -271,11 +280,20 @@ int http_request_builder_send(HTTP_RequestBuilder builder)
|
|||||||
for (int i = 0; i < 8; i++)
|
for (int i = 0; i < 8; i++)
|
||||||
target.ipv6.data[i] = conn->url.authority.host.ipv6.data[i];
|
target.ipv6.data[i] = conn->url.authority.host.ipv6.data[i];
|
||||||
} else {
|
} else {
|
||||||
|
// Invalid host mode - clean up connection
|
||||||
|
http_client_conn_free(conn);
|
||||||
|
conn->state = HTTP_CLIENT_CONN_FREE;
|
||||||
|
conn->client->num_conns--;
|
||||||
return -1;
|
return -1;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (socket_connect(&conn->client->sockets, 1, &target, secure, conn) < 0)
|
if (socket_connect(&conn->client->sockets, 1, &target, secure, conn) < 0) {
|
||||||
|
// Connection failed - clean up
|
||||||
|
http_client_conn_free(conn);
|
||||||
|
conn->state = HTTP_CLIENT_CONN_FREE;
|
||||||
|
conn->client->num_conns--;
|
||||||
return -1;
|
return -1;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
conn->state = HTTP_CLIENT_CONN_FLUSHING;
|
conn->state = HTTP_CLIENT_CONN_FLUSHING;
|
||||||
|
|||||||
+6
-2
@@ -36,6 +36,9 @@ typedef struct {
|
|||||||
// Data being sent to the server
|
// Data being sent to the server
|
||||||
ByteQueue output;
|
ByteQueue output;
|
||||||
|
|
||||||
|
// HTTP method for the request
|
||||||
|
HTTP_Method method;
|
||||||
|
|
||||||
// Parsed URL for connection establishment
|
// Parsed URL for connection establishment
|
||||||
HTTP_URL url;
|
HTTP_URL url;
|
||||||
|
|
||||||
@@ -106,9 +109,10 @@ typedef struct {
|
|||||||
int http_client_get_builder(HTTP_Client *client,
|
int http_client_get_builder(HTTP_Client *client,
|
||||||
HTTP_Response *response, HTTP_RequestBuilder *builder);
|
HTTP_Response *response, HTTP_RequestBuilder *builder);
|
||||||
|
|
||||||
// Set the URL of the current request. This is the first
|
// Set the method and URL of the current request. This is the first
|
||||||
// function of the request builder that the user must call.
|
// function of the request builder that the user must call.
|
||||||
void http_request_builder_url(HTTP_RequestBuilder builder, HTTP_String url);
|
void http_request_builder_url(HTTP_RequestBuilder builder,
|
||||||
|
HTTP_Method method, HTTP_String url);
|
||||||
|
|
||||||
// After the URL, the user may set zero or more headers.
|
// After the URL, the user may set zero or more headers.
|
||||||
void http_request_builder_header(HTTP_RequestBuilder builder, HTTP_String str);
|
void http_request_builder_header(HTTP_RequestBuilder builder, HTTP_String str);
|
||||||
|
|||||||
+1
-1
@@ -435,7 +435,7 @@ static int is_scheme_body(char c)
|
|||||||
// Note: percent-encoded characters (%XX) are not currently validated
|
// Note: percent-encoded characters (%XX) are not currently validated
|
||||||
static int is_userinfo(char c)
|
static int is_userinfo(char c)
|
||||||
{
|
{
|
||||||
return is_unreserved(c) || is_sub_delim(c) || c == ':' || c == '%';
|
return is_unreserved(c) || is_sub_delim(c) || c == ':';
|
||||||
}
|
}
|
||||||
|
|
||||||
// authority = [ userinfo "@" ] host [ ":" port ]
|
// authority = [ userinfo "@" ] host [ ":" port ]
|
||||||
|
|||||||
+1
-1
@@ -240,7 +240,7 @@ bool http_server_next_request(HTTP_Server *server,
|
|||||||
if (server->num_ready == 0)
|
if (server->num_ready == 0)
|
||||||
return false;
|
return false;
|
||||||
|
|
||||||
HTTP_ServerConn *conn = &server->conns[server->ready_head];
|
HTTP_ServerConn *conn = &server->conns[server->ready[server->ready_head]];
|
||||||
server->ready_head = (server->ready_head + 1) % HTTP_SERVER_CAPACITY;
|
server->ready_head = (server->ready_head + 1) % HTTP_SERVER_CAPACITY;
|
||||||
server->num_ready--;
|
server->num_ready--;
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user