Skip to content

Commit

Permalink
Ignore error of udp (#246)
Browse files Browse the repository at this point in the history 
See ZLMediaKit/ZLMediaKit#3807
  • Loading branch information
@xiongguangjie
xiongguangjie authored Sep 17, 2024
1 parent 1b36624 commit c8ea4cc
Showing 1 changed file with 5 additions and 1 deletion.
6 changes: 5 additions & 1 deletion src/Network/Socket.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,11 @@ bool Socket::attachEvent(const SockNum::Ptr &sock) {
strong_self->onWriteAble(sock);
}
if (event & EventPoller::Event_Error) {
strong_self->emitErr(getSockErr(sock->rawFd()));
if (sock->type() == SockNum::Sock_UDP) {
// udp ignore error
} else {
strong_self->emitErr(getSockErr(sock->rawFd()));
}
}
});

Expand Down

1 comment on commit c8ea4cc

@alexliyu7352
Copy link
Member

Choose a reason for hiding this comment

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

AI Review for src/Network/Socket.cpp:

Code Review: Patch to src/Network/Socket.cpp

Summary

This patch addresses the issue of UDP sockets not handling errors properly. It introduces a conditional check to ignore errors for UDP sockets, while still emitting errors for TCP sockets.

Detailed Feedback

Code Overview

The patch adds a conditional check within the attachEvent function to determine the socket type. If the socket is UDP, errors are ignored. Otherwise, the emitErr function is called to handle the error.

Strengths

  • Targeted Fix: The patch directly addresses the specific issue of UDP error handling.
  • Clear Logic: The conditional check is straightforward and easy to understand.
  • Concise Code: The patch is minimal and only modifies the relevant section of the code.

Areas for Improvement

1. Error Handling Philosophy

  • Issue: The current approach of ignoring UDP errors might mask potential issues. While UDP is inherently unreliable, it's still beneficial to log or handle errors for debugging and monitoring purposes.
  • Suggestion: Instead of completely ignoring UDP errors, consider logging them with a warning level. This would provide valuable information without disrupting the normal UDP operation.
  • Example: 
    if (event & EventPoller::Event_Error) {
    if (sock->type() == SockNum::Sock_UDP) {
        // udp ignore error, but log it
        WarnL << "UDP socket error: " << getSockErr(sock->rawFd());
    } else {
        strong_self->emitErr(getSockErr(sock->rawFd()));
    }
}

2. Documentation

  • Issue: The comment "udp ignore error" is not very informative.
  • Suggestion: Add a more detailed comment explaining the rationale behind ignoring UDP errors and the potential implications.
  • Example: 
    if (event & EventPoller::Event_Error) {
    if (sock->type() == SockNum::Sock_UDP) {
        // UDP sockets are inherently unreliable and errors are common.
        // Ignoring errors here prevents unnecessary disruption of UDP communication.
        // However, it's important to note that errors might indicate underlying issues.
        // Consider logging errors for debugging and monitoring purposes.
    } else {
        strong_self->emitErr(getSockErr(sock->rawFd()));
    }
}

Conclusion

The patch effectively addresses the UDP error handling issue. However, the approach of completely ignoring errors might be too aggressive. Logging errors with a warning level would provide a better balance between robustness and debugging capabilities. Additionally, improving the documentation would enhance code clarity and maintainability.

TRANS_BY_GITHUB_AI_ASSISTANT

Please sign in to comment.