Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add debugging log when error occurs in Accept() #222

Merged
merged 5 commits into from
Jul 10, 2021

Conversation

hellodudu
Copy link
Contributor


name: Pull request
about: wrap errors description
title: 'wrap errors'
labels: ''
assignees: ''

1. Are you opening this pull request for bug-fixs, optimizations or new feature?

optimizations

2. Please describe how these code changes achieve your intention.

acceptNewConnection sometimes return a simple ErrAcceptSocket error with no exactly description. It will cost so much time to figure out where the problem is. For example, if the connection number achieved system's fd limit, I just got an error message like Main reactor is exiting due to error: accept a new connection error. After errors wrapped, the error message is like Main reactor is exiting due to error: accept a new connection error: too many open files.

3. Please link to the relevant issues (if any).

4. Which documentation changes (if any) need to be made/updated because of this PR?

4. Checklist

  • I have squashed all insignificant commits.
  • I have commented my code for explaining package types, values, functions, and non-obvious lines.
  • I have written unit tests and verified that all tests passes (if needed).
  • I have documented feature info on the README (only when this PR is adding a new feature).
  • (optional) I am willing to help maintain this change if there are issues with it later.

@panjf2000
Copy link
Owner

Sorry, I don't see any value from this code change, what's the point of this PR?

@panjf2000 panjf2000 added the waiting for response waiting for the response from commenter label Jul 8, 2021
@hellodudu
Copy link
Contributor Author

I got an accept a new connection error when benchmark gnet, but I don't know how it comes until adding logs into acceptNewConnection
I think wrapping all returned errors into err and use errors.Is() to check if it contains any error is benefit for positioning problem, though it's less graceful.

@panjf2000
Copy link
Owner

If you're just doing this only for debugging purposes, I'd suggest you add a debug log under this:

logging.Infof("Main reactor is exiting due to error: %v", err)

@hellodudu
Copy link
Contributor Author

logging.Infof("Main reactor is exiting due to error: %v", err)

I'm afraid this err just returns errors.ErrAcceptSocket with no more details when unix.Accept(fd) failed. And we won't know why acceptNewConnection fails unless adding some logs before return errors.ErrAcceptSocket.

gnet/acceptor_unix.go

Lines 34 to 40 in b4a9840

nfd, sa, err := unix.Accept(fd)
if err != nil {
if err == unix.EAGAIN {
return nil
}
return errors.ErrAcceptSocket
}

@panjf2000
Copy link
Owner

logging.Infof("Main reactor is exiting due to error: %v", err)

I'm afraid this err just returns errors.ErrAcceptSocket with no more details when unix.Accept(fd) failed. And we won't know why acceptNewConnection fails unless adding some logs before return errors.ErrAcceptSocket.

gnet/acceptor_unix.go

Lines 34 to 40 in b4a9840

nfd, sa, err := unix.Accept(fd)
if err != nil {
if err == unix.EAGAIN {
return nil
}
return errors.ErrAcceptSocket
}

Make senses, then could you take a look across acceptor_unix.go, connection_unix.go and eventloop_unix.go and add logging.Debugf or logging.Warnf for those critical errors?
@hellodudu

@panjf2000 panjf2000 changed the title wrap errors chore: add more debug logs for critical errors Jul 9, 2021
@panjf2000 panjf2000 changed the title chore: add more debug logs for critical errors chore: add more debugging logs for critical errors Jul 9, 2021
@hellodudu
Copy link
Contributor Author

You mean adding logs instead of wrap errors? sure, that's better.

@panjf2000
Copy link
Owner

I just refactored the logging logic in 8837a92, please rebase your branch on it.
@hellodudu

@hellodudu
Copy link
Contributor Author

Done.

@codecov
Copy link

codecov bot commented Jul 10, 2021

Codecov Report

Merging #222 (3861b6e) into master (8837a92) will increase coverage by 1.83%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #222      +/-   ##
==========================================
+ Coverage   83.83%   85.67%   +1.83%     
==========================================
  Files          18       18              
  Lines        1231     1410     +179     
==========================================
+ Hits         1032     1208     +176     
- Misses        148      151       +3     
  Partials       51       51              
Flag Coverage Δ
unittests 85.67% <0.00%> (+1.83%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
acceptor_unix.go 41.66% <0.00%> (+0.75%) ⬆️
loop_bsd.go 100.00% <0.00%> (ø)
listener_unix.go 100.00% <0.00%> (ø)
acceptor_windows.go 100.00% <0.00%> (ø)
options.go 88.67% <0.00%> (+0.10%) ⬆️
server_windows.go 98.05% <0.00%> (+0.20%) ⬆️
reactor_bsd.go 87.50% <0.00%> (+0.83%) ⬆️
server_unix.go 88.95% <0.00%> (+0.95%) ⬆️
listener_windows.go 93.33% <0.00%> (+1.02%) ⬆️
eventloop_windows.go 90.78% <0.00%> (+1.23%) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8837a92...3861b6e. Read the comment docs.

@panjf2000 panjf2000 changed the title chore: add more debugging logs for critical errors chore: add debugging log when error occurs in Accept() Jul 10, 2021
acceptor_unix.go Outdated Show resolved Hide resolved
acceptor_unix.go Outdated Show resolved Hide resolved
Copy link
Owner

@panjf2000 panjf2000 left a comment

Choose a reason for hiding this comment

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

Thanks~

@panjf2000 panjf2000 added pending merged This PR has been reviewed and approved and removed waiting for response waiting for the response from commenter labels Jul 10, 2021
@panjf2000 panjf2000 merged commit f718aef into panjf2000:master Jul 10, 2021
0-haha pushed a commit to 0-haha/gnet that referenced this pull request Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending merged This PR has been reviewed and approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants