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

Cleanup session.Manager's handling of errors. #122

Merged
merged 2 commits into from
Jun 26, 2020

Conversation

jmuk
Copy link
Contributor

@jmuk jmuk commented Jun 24, 2020

It turns out that cdp.ErrorCause tracks the list of causes and
returns the true cause of the problem. In the case of bug #120,
the topmost err was internal.OpError, and the ErrorCause returns
underlying websocket.CloseError.

Actually there's rpcc.closeError in the middle of cause chain,
but that's simply skipped, and therefore it fails to recognize
the error as closing.

This tracks of every step of Cause and return true if any of
these errors are closing one.

jmuk and others added 2 commits June 24, 2020 16:09
It turns out that cdp.ErrorCause tracks the list of causes and
returns the true cause of the problem. In the case of bug mafredri#120,
the topmost err was internal.OpError, and the ErrorCause returns
underlying websocket.CloseError.

Actually there's rpcc.closeError in the middle of cause chain,
but that's simply skipped, and therefore it fails to recognize
the error as closing.

This tracks of every step of Cause and return true if any of
these errors are closing one.
Copy link
Owner

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

Ah, this makes sense, good catch.

I have a go2errors branch that has been all but forgotten. The Unwrap (or perhaps As/Is APIs) would be a good fit for this.

But this is a great fix, so let's get this merged now!

@codecov
Copy link

codecov bot commented Jun 26, 2020

Codecov Report

Merging #122 into master will increase coverage by 0.2%.
The diff coverage is 100.0%.

@@           Coverage Diff            @@
##           master    #122     +/-   ##
========================================
+ Coverage    85.6%   85.8%   +0.2%     
========================================
  Files          20      20             
  Lines        1019    1020      +1     
========================================
+ Hits          872     875      +3     
+ Misses        114     112      -2     
  Partials       33      33             

@mafredri mafredri merged commit 86e3bb8 into mafredri:master Jun 26, 2020
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.

2 participants