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.

Sign up for GitHub

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

Jump to bottom

irc: Improve output for common connection errors #2430

Merged
merged 5 commits into from
May 23, 2023
Merged

irc: Improve output for common connection errors #2430

merged 5 commits into from
May 23, 2023

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented Mar 23, 2023

Description

Closes #2111 and open the way for better error handling and connection retry.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

@Exirel Exirel added this to the 8.0.0 milestone Mar 23, 2023
dgw
dgw requested changes Mar 24, 2023
View reviewed changes
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

<+xrl> dgw: the draft is to see if you want more out of our error logs

I like this. I think it's more than sufficient to close #2111, and we can keep an eye out for more improvements that can be made to 8.1. Just a couple tweaks. 🙂

sopel/irc/__init__.py Outdated Show resolved Hide resolved
sopel/irc/backends.py Outdated Show resolved Hide resolved
sopel/irc/backends.py Outdated Show resolved Hide resolved
Exirel and others added 2 commits March 24, 2023 22:23
@Exirel @dgw
irc: deprecate AbstractBot.on_error
753a39e 
This method was used for the asynchat backend implementation.
It is now deprecated and must not be used.

Co-authored-by: dgw <dgw@technobabbl.es>
@Exirel
irc: user-friendly messages on connection error
b6c5e86 
These error are now managed with a more user-friendly message, and the
exception is sent to the exception log file:

* SSL certificate validation error
* SSL generic error
* Connection timeout (socket timeout, not IRC timeout)
* Invalid hostname
* Unable to connect (temporary disconnected, host unreachable, etc.)

If we inspect the OSError's errno we could provide a more fine-grained
error handling.

We could, also, improve the connection retry for certain errors.
@Exirel Exirel force-pushed the common-connection-errors-output branch from 02e07a4 to b6c5e86 Compare March 24, 2023 21:28
@Exirel Exirel marked this pull request as ready for review March 24, 2023 21:28
@Exirel
Copy link
Contributor Author

Exirel commented Mar 24, 2023

Alright, then it's ready for an actual true review. 😎

I expect some nitpicking on the wording and all that! 😁

SnoopJ
SnoopJ requested changes Mar 24, 2023
View reviewed changes
sopel/irc/backends.py Outdated Show resolved Hide resolved
sopel/irc/backends.py Show resolved Hide resolved
sopel/irc/backends.py Show resolved Hide resolved
sopel/irc/backends.py Show resolved Hide resolved
@Exirel Exirel mentioned this pull request Mar 24, 2023
Merged
4 tasks
@Exirel @SnoopJ
irc: friendler message on certificate verification failure
3a81afa 
Co-authored-by: James Gerity <snoop.jedi@gmail.com>
@Exirel Exirel force-pushed the common-connection-errors-output branch from 0900e79 to 1116918 Compare April 8, 2023 20:49
@Exirel Exirel requested review from SnoopJ and dgw April 8, 2023 22:15
@Exirel
Copy link
Contributor Author

Exirel commented Apr 8, 2023

@SnoopJ @dgw did the thing to handle even more cases!

dgw
dgw reviewed Apr 11, 2023
View reviewed changes
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

I just have nitpicks.

sopel/irc/backends.py Outdated Show resolved Hide resolved
sopel/irc/backends.py Show resolved Hide resolved
sopel/irc/backends.py Outdated Show resolved Hide resolved
@Exirel Exirel requested a review from dgw April 19, 2023 08:33
Exirel and others added 2 commits April 19, 2023 10:34
@Exirel @dgw
irc: separate ConnectionError from OSError
96a52ac 
Co-authored-by: dgw <dgw@technobabbl.es>
@Exirel @dgw
irc: isolate the socket connection attempt
c4a34f7 
Co-authored-by: dgw <dgw@technobabbl.es>
@Exirel Exirel force-pushed the common-connection-errors-output branch from f174b65 to c4a34f7 Compare April 19, 2023 08:35
@Exirel
Copy link
Contributor Author

Exirel commented Apr 19, 2023

@dgw nitpick fixed.

@dgw dgw changed the title Common connection errors output irc: Improve output for common connection errors Apr 20, 2023
dgw
dgw approved these changes Apr 20, 2023
View reviewed changes
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

I think this is ready as it'll ever be from my perspective. Just curious if @SnoopJ considers that connection block in irc/backends.py to be fine now; I saw he suggested abstracting a bunch of error handling into a method call, but it doesn't look like that was done before resolving the thread?

@Exirel
Copy link
Contributor Author

Exirel commented Apr 20, 2023

I saw he suggested abstracting a bunch of error handling into a method call

I did with a method _connect_to_server that is used for self._reader, self._writer = self._connect_to_server(...).

@dgw
Copy link
Member

dgw commented Apr 20, 2023

@Exirel Ah, so you did. Got confused by the line anchoring that discussion thread being inside the new method, untouched. 😅

SnoopJ
SnoopJ approved these changes Apr 20, 2023
View reviewed changes
Copy link
Contributor

@SnoopJ SnoopJ left a comment

Choose a reason for hiding this comment

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

I had a nit, but LGTM 👍

sopel/irc/backends.py Show resolved Hide resolved
This was referenced May 2, 2023
Closed
Merged
@dgw dgw merged commit ccb6138 into sopel-irc:master May 23, 2023
@dgw
Copy link
Member

dgw commented May 23, 2023

Given #2455, merging this is risking it for the biscuit a little, but I really want to start shipping stuff again.

@dgw dgw linked an issue May 29, 2023 that may be closed by this pull request
socket.gaierror during (re)connection is unhandled #2407
Closed
@dgw dgw mentioned this pull request May 29, 2023
Closed
@Exirel Exirel deleted the common-connection-errors-output branch June 15, 2023 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SnoopJ SnoopJ SnoopJ approved these changes

@dgw dgw dgw approved these changes

Assignees
No one assigned
Labels
Core/Networking Tweak
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

socket.gaierror during (re)connection is unhandled Common connection errors result in tracebacks
3 participants
@Exirel @dgw @SnoopJ