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

Sending .close(status=4000) results in an error #639

Closed
elderlabs opened this issue Sep 30, 2020 · 4 comments
Closed

Sending .close(status=4000) results in an error #639

elderlabs opened this issue Sep 30, 2020 · 4 comments

Comments

@elderlabs
Copy link

elderlabs commented Sep 30, 2020

I maintain a library to interface with Discord's API. Some time ago, they made a change to help balance out connections in their auto-scaling hardware in-use. The issue: they require you send a status code in your WS closure in order to specify whether you're reconnecting or disconnecting -- as their API sends reconnect OPCodes at intervals. When specifying the status code to send back after the OPCode, websocket_client logs the closure as an error, thus triggering Sentry, our error logger/notification dispatcher.

The questions here is whether there's some way to silence this error, because it's not an error. Everything's working as intended on our end, thus the status code shouldn't trigger an error at all. Perhaps a logging flag to go with the close to specify whether it should log as an error, or log as info, but for compatibility's sake default to logging as an error as that's the current behavior.

I noticed something mentioned in #414, but it didn't seem to really touch on the issue.

@engn33r
Copy link
Collaborator

engn33r commented Mar 5, 2021

@elderlabs I think I understand what you're getting at, but let me try to summarize with my own words to confirm.

websocket-client allows ws.close(status=4000) to close a connection with a specific status code and the program exits/runs cleanly. The issue you describe stems from this line, which calls the error() function in _logging.py for any opcode other than STATUS_NORMAL. I think the original intent of this was that the defined status codes 1001-1015 other than STATUS_NORMAL, seen here in RFC6455, are related to various error conditions. However, the if recv_status != STATUS_NORMAL check currently in use does not consider the custom status codes 3000-4999, which are not necessarily errors, since they are custom defined.

I can add an additional if branch for opcodes 3000-4999, but this brings up a question I think you should help answer. If you are using Sentry and do not want it to trigger in a .close(status=4000) condition, which _logging.py function should be used as a replacement for error()? I think debug() would be my pick, but I want to get your input before making the change.

@elderlabs
Copy link
Author

Thank you for your reply. Some time ago, we learned how to suppress the error once it reached Sentry's logging mechanism before dispatching the error to their API.

If it were up to me, debug() would be a good solution to the issue. I did find that line in _logging.py while studying the issue, but opted to properly silence the issue in Sentry on my end.

Thank you for your time and consideration.

@engn33r engn33r closed this as completed in df87514 Mar 8, 2021
@engn33r
Copy link
Collaborator

engn33r commented Mar 8, 2021

I am glad the issue was resolved in Sentry, but I still thought I would add this minor edit as a small improvement

@elderlabs
Copy link
Author

Considering I can now remove the suppressor we hard-coded into the Sentry-logging function on our end, this is a welcome change. Sorry if I didn't clarify that in my original reply. Thank you!

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

No branches or pull requests

2 participants