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

Tear down assocation on ABORT chunk #102

Merged
merged 3 commits into from
Feb 1, 2020
Merged

Tear down assocation on ABORT chunk #102

merged 3 commits into from
Feb 1, 2020

Conversation

Sean-Der
Copy link
Member

Properly tear down the association when we receive an ABORT
chunk.

@Sean-Der Sean-Der requested a review from enobufs January 31, 2020 09:27
@Sean-Der
Copy link
Member Author

@enobufs I am going to write a test to confirm this doesn't regress as well, but ran out of time tonight!

@@ -1963,8 +1965,8 @@ func (a *Association) handleChunk(p *packet, c chunk) error {
var err error

if _, err = c.check(); err != nil {
return errors.Wrap(err, "failed validating chunk")
// TODO: Create ABORT
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't find anything in the RFC, but it seems like we should tolerate anything from the other side (besides ABORT chunks)

Copy link
Member

@enobufs enobufs Jan 31, 2020

Choose a reason for hiding this comment

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

There are some cases mentioned in RFC 4960 which require sending ABORT to close the association. (grep by should abort or must abort in the RFC).
Chunks that can return an error from check() in the current code are chunk_init.go and chunk_init_ack.go as far as I can tell.
We've been ignoring those errors so the logic hasn't be proven. I'd support your changes (just log and ignore) for now to reduce risk of regression. We could revisit this later, maybe when we finally support "sending ABORT".

Copy link
Member Author

Choose a reason for hiding this comment

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

Ticketed! #103

@codecov-io
Copy link

codecov-io commented Jan 31, 2020

Codecov Report

Merging #102 into master will increase coverage by 1.57%.
The diff coverage is 65.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
+ Coverage   77.05%   78.62%   +1.57%     
==========================================
  Files          43       43              
  Lines        2711     2723      +12     
==========================================
+ Hits         2089     2141      +52     
+ Misses        495      452      -43     
- Partials      127      130       +3
Flag Coverage Δ
#go 78.62% <65.21%> (+1.57%) ⬆️
#wasm 70.54% <34.78%> (+0.75%) ⬆️
Impacted Files Coverage Δ
association.go 83.14% <57.14%> (+0.62%) ⬆️
chunk_abort.go 59.37% <77.77%> (+59.37%) ⬆️
packet.go 65.85% <0%> (+2.43%) ⬆️
error_cause_header.go 100% <0%> (+10.52%) ⬆️
error_cause.go 33.33% <0%> (+17.77%) ⬆️
error_cause_invalid_mandatory_parameter.go 33.33% <0%> (+33.33%) ⬆️
error_cause_protocol_violation.go 81.81% <0%> (+81.81%) ⬆️

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 879b876...10fdba6. Read the comment docs.

Copy link
Member

@enobufs enobufs left a comment

Choose a reason for hiding this comment

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

Your changes look good to me.

@Sean-Der Sean-Der requested a review from enobufs February 1, 2020 00:02
Properly tear down the association when we receive an ABORT
chunk.
Add test to assert that sending an abort takes the assocation
to closed.
@enobufs
Copy link
Member

enobufs commented Feb 1, 2020

@Sean-Der Just to be sure that it can handle multiple errorCauses, I have added a marshaling test. I think your changes look good!

Copy link
Member

@enobufs enobufs left a comment

Choose a reason for hiding this comment

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

LGTM

@Sean-Der
Copy link
Member Author

Sean-Der commented Feb 1, 2020

@enobufs thank you so much. I appreciate how much work you have put forward :)

@Sean-Der Sean-Der merged commit 7219116 into master Feb 1, 2020
@Sean-Der Sean-Der deleted the abort-teardown branch February 1, 2020 01:16
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.

3 participants