-
Notifications
You must be signed in to change notification settings - Fork 155
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
Fix typing for alertErrors #627
Conversation
I think there's some problem with CI |
@sukunrt can you open this against the master branch? The linter configs are global, and I haven't been updating the sorry about that! |
conn.go
Outdated
return e | ||
} | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure this makes sense? We're first checking if it's a particular error with a particular condition and then return e
, but that's still of type error
. Right after, we're doing if err != nil { return err }
. It seems at that point we don't need the check for if errors.As
at all, since we always return err
if it's not nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right. I copied this from master.
I think all we need is if err != nil {return err}
because the caller checks for IsFatalOrCloseNotify
. I've updated the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing that. I think you might've forgotten to push the right branch? I don't see the change and CI hasn't picked up on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the else if err != nil
in place of if err != nil
to keep the current behavior as is.
I believe the intent here is to patch the v2 release to allow a patch release with this change. This allows users to get this fix with no changes on their end besides updating dtls by a patch version. My understanding is that master doesn't have this issue, but cutting a release from master is a bit more involved and requires changes in pion/webrtc as well in order for users to get the fix. |
@MarcoPolo @sukunrt ok great, will review! You can ignore the lint/codecov failures. It is just an artifact of the CI not being stateless :/ |
e39f4a4
to
60a3e4a
Compare
@daenney, I've updated the PR now. I made one change after this comment. #627 (comment) |
60a3e4a
to
1e07d06
Compare
@@ -648,7 +643,7 @@ func (c *Conn) handleQueuedPackets(ctx context.Context) error { | |||
return e | |||
} | |||
} else if err != nil { | |||
return e | |||
return err |
There was a problem hiding this 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 the same situation as on L610, the additional checking for alertError
doesn't do anything. But if this exists on master still then lets fix that there instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. I didn't want to change any behavior so went for the minimal change.
On line 610 the caller checks whether alertError is fatal so we dont need the check here and just return err if it isn't nil.
The caller for line 651 doesn't check if the alertError is fatal so we check it here and suppress alertErrors that are not fatal.
ffddd3f
to
767143c
Compare
No description provided.