Skip to content

Conversation

@ruslanti
Copy link
Contributor

@ruslanti ruslanti commented Jul 1, 2025

No description provided.

@ruslanti ruslanti requested review from Copilot and gabioprisan July 1, 2025 09:53
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds guard checks for NGINX shutdown flags before scheduling event timers in the Unix socket logic and introduces a new uri_method accessor in HTTP requests.

  • Added conditional guards (ngx_quit, ngx_exiting, ngx_terminate, and timer_set) around ngx_event_add_timer calls in several places to prevent scheduling timers during shutdown or if already set.
  • Exposed HTTP method name via a new pub fn uri_method(&self) -> NgxStr in HttpRequest.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
unix_socket.rs Wrapped ngx_event_add_timer calls with shutdown and timer_set checks in three places
http_request.rs Added a new public uri_method method returning NgxStr from the native method_name
Comments suppressed due to low confidence (2)

nginx_module/src/http_request.rs:303

  • [nitpick] The name uri_method may be misleading since it returns the HTTP method (e.g., GET or POST), not a URI. Consider renaming to http_method or simply method for clarity.
    pub fn uri_method(&self) -> NgxStr {

nginx_module/src/http_request.rs:303

  • [nitpick] This new public method lacks a doc comment. Please add a brief description and example usage in the module’s documentation to maintain consistency.
    pub fn uri_method(&self) -> NgxStr {

Comment on lines +86 to +92
if (*ev).timer_set() == 0
&& ngx_quit == 0
&& ngx_exiting == 0
&& ngx_terminate == 0
{
ngx_event_add_timer(&mut *ev, MIN_TIMEOUT_MS);
}
Copy link

Copilot AI Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The timer-setup guard logic is duplicated across multiple methods. Consider extracting this into a helper like add_timer_if_allowed(ev: &mut ngx_event_t, timeout: u64) to reduce repetition and improve readability.

Suggested change
if (*ev).timer_set() == 0
&& ngx_quit == 0
&& ngx_exiting == 0
&& ngx_terminate == 0
{
ngx_event_add_timer(&mut *ev, MIN_TIMEOUT_MS);
}
add_timer_if_allowed(&mut *ev, MIN_TIMEOUT_MS);

Copilot uses AI. Check for mistakes.
@ruslanti ruslanti merged commit 54ddc6d into main Jul 1, 2025
1 check passed
@ruslanti ruslanti deleted the 34-nginx-crash-on-stopping-fastedge branch July 1, 2025 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants