Skip to content

Conversation

@pasis
Copy link
Member

@pasis pasis commented Sep 28, 2025

Description

EOF is a regular scenario. Treating it as an error in the stats can be confusing.

What

Don't treat TCP EOF as an error.

Why ?

Avoid confusion and noise in stats.

Change type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Tests
  • Other

Check list

  • Code follows the style de facto guidelines of this project
  • Comments have been inserted in hard to understand places
  • Documentation has been updated (if necessary)
  • Test has been added (if possible)

@pasis pasis added the draft Not to review yet label Sep 28, 2025
@pasis pasis changed the title issue: xxx Don't increase error counter on TCP EOF issue: 4669942 Don't increase error counter on TCP EOF Oct 20, 2025
@pasis
Copy link
Member Author

pasis commented Oct 20, 2025

bot:retest

@pasis pasis removed the draft Not to review yet label Oct 21, 2025
EOF is a regular scenario. Treating it as an error in the stats can be
confusing.

Signed-off-by: Dmytro Podgornyi <[email protected]>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

Fixed TCP EOF handling to prevent it from being counted as an error in socket statistics. EOF is a normal connection termination scenario and should not inflate error counters.

Key Changes:

  • Modified handle_rx_error() to only increment error counters when ret == -1
  • TCP EOF returns ret = 0, which now correctly bypasses error statistics
  • Updated comments in tcp_in.c for improved clarity (no functional changes)

Impact:

  • Statistics will now accurately reflect actual errors vs normal connection closures
  • Consistent with POSIX behavior where EOF (return 0) is distinct from errors (return -1)

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The change is minimal, well-targeted, and correct. It adds a simple condition (&& ret == -1) to ensure error counters are only incremented for actual errors, not EOF. The logic properly distinguishes between EOF (ret=0) and errors (ret=-1), which aligns with standard POSIX semantics. The change is consistent with similar patterns in the codebase (e.g., sockinfo.cpp:785).
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
src/core/sock/sockinfo_tcp.cpp 5/5 Fixed error counter logic to exclude EOF (ret=0) from error statistics

Sequence Diagram

sequenceDiagram
    participant App as Application
    participant SI as sockinfo_tcp
    participant Stats as Socket Stats
    
    App->>SI: recv() call on TCP socket
    SI->>SI: handle_rx_error(blocking)
    
    alt Connection Closed (EOF)
        SI->>SI: ret = 0 (EOF)
        Note over SI: No errno set
    else Other Errors
        SI->>SI: ret = -1
        Note over SI: errno set (EAGAIN, ECONNRESET, etc)
    end
    
    alt ret == -1 (Error)
        SI->>Stats: Increment n_rx_eagain or n_rx_errors
    else ret == 0 (EOF)
        Note over SI,Stats: No error counter incremented
    end
    
    SI->>App: Return ret (0 for EOF, -1 for error)
Loading

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@galnoam galnoam merged commit a698fb7 into Mellanox:vNext Nov 12, 2025
1 check passed
@galnoam
Copy link
Collaborator

galnoam commented Nov 12, 2025

@pasis , please move to fixed/closed

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.

4 participants