Implement shutdown sequence #176

jeremija · 2020-12-20T20:30:05Z

Description

I finally had some time to continue the work on adding the multi server node support on Peer Calls. I noticed there was an issue where the other side did not notice that a SCTP association was closed from the other side.

I tried to follow the steps from the SCTP Association State Diagram and Shutdown of an Association sections of RFC 4960.

I added two new tests and all existing tests passed.

Instead of modifying the existing Close function I added a new Shutdown method to keep the backwards compatibility. I also thought it made sense to allow the users to pick which teardown strategy to use.

Perhaps the Close should be modified to use Abort?

Reference issue

Fixes #152.

codecov · 2020-12-20T20:30:58Z

Codecov Report

Merging #176 (bfc8c8f) into master (7017b05) will increase coverage by 0.48%.
The diff coverage is 81.52%.

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
+ Coverage   77.35%   77.84%   +0.48%     
==========================================
  Files          43       46       +3     
  Lines        2438     2609     +171     
==========================================
+ Hits         1886     2031     +145     
- Misses        419      440      +21     
- Partials      133      138       +5

Flag	Coverage Δ
go	`77.84% <81.52%> (+0.48%)`	⬆️
wasm	`65.46% <32.06%> (-2.59%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
association.go	`81.22% <78.98%> (-0.45%)`	⬇️
chunk_shutdown.go	`80.00% <80.00%> (ø)`
chunk_shutdown_ack.go	`88.88% <88.88%> (ø)`
chunk_shutdown_complete.go	`88.88% <88.88%> (ø)`
packet.go	`67.90% <100.00%> (+2.56%)`	⬆️
stream.go	`94.66% <100.00%> (+1.65%)`	⬆️
... and 2 more

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 7017b05...096b349. Read the comment docs.

Sean-Der · 2020-12-25T03:40:24Z

Oh wow this is amazing! Nice work @jeremija sorry I got distracted by v3.0.0 :(

Reviewing now @enobufs would love your help (if you can!) you know this way better then me.

enobufs · 2021-02-13T21:36:30Z

My apologies for not being able spare time for this. I will review this from now. I have just rebased on current master. It appears to be such an excellent work indeed! :D

enobufs

This is really an amazing work! Thanks so much for being one of the very few contributors of SCTP. I have left some comments. Also, let me share my notes just in case it helps.
Thanks again!

association.go

enobufs · 2021-02-14T01:56:24Z

association.go

+
+	a.lock.Unlock()
+
+	return a.closeWriteLoopCh, nil


I am a bit nervous about returning the channel (though it's read-only). May I hear your thoughts on this?

I think I just wanted a way for the caller to know when the shutdown process has completed since it does not happen in an instant like Close does.

Shutdown sequence (esp. graceful one) is always a headache...
I personally feel that the Shutdown() should block on the channel by itself. If necessary, caller can always choose to call Shutdown() from a goroutine for async operation. Hmm, let me ask Sean.

@Sean-Der What is your thought on this? The Shutdown() method triggers the shutdown sequence. Should it block until the sequence is complete, or return a channel so that the caller could choose to wait on the channel...?
Pion mixes Javascript (non-blocking) and Go (blocking) styles which confuses myself sometimes...

I think blocking is the way to go. You can optionally pass in a context to be notified. I am not an expert Go developer either though :)

crypto/tls is a blocking Close

HTTP provides two APIs Context and Blocking

net.Conn is blocking as well.

So excited to have you both back I missed working together! I learn so much from your PRs and so fun to see Pion progress so quickly without me :)

Thanks, I like the context.Context way. I just pushed an update that modifies the sctp.Association.Shutdown's signature to:

// Shutdown initiates the shutdown sequence. The method blocks until the // shutdown sequence is completed and the connection is closed, or until the // passed context is done, in which case the context's error is returned. func (a *Association) Shutdown(ctx context.Context) error {

Does that work?

So excited to have you both back I missed working together! I learn so much from your PRs and so fun to see Pion progress so quickly without me :)

Aww thanks ❤️ I miss Pion and working with you, I learned so much from the whole community and I don't think I'd have the clients I currently have if it weren't for you guys!

jeremija · 2021-02-17T07:35:07Z

Thanks for the kind words and the review! No worries about the time - this is open source after all! I've lost a little bit of context, but I'll do my best to answer your questions 🙂

Closes #152.

enobufs

@Sean-Der thanks for your comment. @jeremija I like the use of context too, more idiomatic in Go. I can also see that when we improve the detached datachannel (#77), and introduction of Listen/Accept (#74), this Shutdown method with context will become very useful. At that point, as @jeremija pointed out, we'd consider renaming Shutdown to Close and the current Close to Abort, etc. Let me approve this.

jeremija · 2021-02-20T07:26:33Z

Thanks for your review @enobufs!

we'd consider renaming Shutdown to Close and the current Close to Abort

Actually I think the current implementation of Close is not implemented as per the specification for Abort:

9.1.  Abort of an Association

   When an endpoint decides to abort an existing association, it MUST
   send an ABORT chunk to its peer endpoint.  The sender MUST fill in
   the peer's Verification Tag in the outbound packet and MUST NOT
   bundle any DATA chunk with the ABORT.  If the association is aborted
   on request of the upper layer, a User-Initiated Abort error cause
   (see Section 3.3.10.12) SHOULD be present in the ABORT chunk.

   An endpoint MUST NOT respond to any received packet that contains an
   ABORT chunk (also see Section 8.4).

   An endpoint receiving an ABORT MUST apply the special Verification
   Tag check rules described in Section 8.5.1.

   After checking the Verification Tag, the receiving endpoint MUST
   remove the association from its record and SHOULD report the
   termination to its upper layer.  If a User-Initiated Abort error
   cause is present in the ABORT chunk, the Upper Layer Abort Reason
   SHOULD be made available to the upper layer.

According to my understanding, we don't send any chunk after Close is called, we just exit the write goroutine. But I might as well be missing something here.

Also, are we currently doing any checks for the verification tags when we receive packets? I see only assertions being done in tests, but not this:

   When receiving an SCTP packet, the endpoint MUST ensure that the
   value in the Verification Tag field of the received SCTP packet
   matches its own tag.  If the received Verification Tag value does not
   match the receiver's own tag value, the receiver shall silently
   discard the packet and shall not process it any further except for
   those cases listed in Section 8.5.1 below.

According to RFC 4960. Also see my comment at #176. Closes #182. I don't think we currently do any tag verification for any packets, but we can implement that later.

jeremija force-pushed the jeremija/issue-152-shutdown-seq branch from 7d59e0d to d906422 Compare December 20, 2020 20:30

jeremija force-pushed the jeremija/issue-152-shutdown-seq branch 2 times, most recently from 1e03c34 to d3526d0 Compare December 20, 2020 22:08

jeremija requested a review from Sean-Der December 20, 2020 22:11

Sean-Der requested a review from enobufs December 25, 2020 03:40

enobufs force-pushed the jeremija/issue-152-shutdown-seq branch from d3526d0 to 4805ca3 Compare February 13, 2021 21:31

enobufs reviewed Feb 14, 2021

View reviewed changes

enobufs mentioned this pull request Feb 14, 2021

sctp ERROR: ... stream 3 not found / with Firefox #150

Closed

jeremija force-pushed the jeremija/issue-152-shutdown-seq branch from 4805ca3 to 85af742 Compare February 17, 2021 07:45

Implement shutdown sequence

096b349

Closes #152.

jeremija force-pushed the jeremija/issue-152-shutdown-seq branch from 85af742 to 096b349 Compare February 18, 2021 07:50

enobufs self-requested a review February 20, 2021 05:51

enobufs approved these changes Feb 20, 2021

View reviewed changes

jeremija merged commit 04897bc into master Feb 20, 2021

jeremija added a commit that referenced this pull request Feb 20, 2021

Add sctp.Association.Abort(reason string) method

cd4051f

According to RFC 4960. Also see my comment at #176. Closes #182. I don't think we currently do any tag verification for any packets, but we can implement that later.

jeremija mentioned this pull request Feb 20, 2021

Add sctp.Association.Abort(reason string) method #183

Merged

jeremija added a commit that referenced this pull request Feb 20, 2021

Add sctp.Association.Abort(reason string) method

ddb16a6

According to RFC 4960. Also see my comment at #176. Closes #182. I don't think we currently do any tag verification for any packets, but we can implement that later.

jeremija added a commit that referenced this pull request Feb 20, 2021

Add sctp.Association.Abort(reason string) method

c3b10d2

According to RFC 4960. Also see my comment at #176. Closes #182. I don't think we currently do any tag verification for any packets, but we can implement that later.

jeremija added a commit that referenced this pull request May 6, 2022

Add sctp.Association.Abort(reason string) method

9299e2c

According to RFC 4960. Also see my comment at #176. Closes #182. I don't think we currently do any tag verification for any packets, but we can implement that later.

jeremija added a commit that referenced this pull request May 10, 2022

Add sctp.Association.Abort(reason string) method

cf3a11a

According to RFC 4960. Also see my comment at #176. Closes #182. I don't think we currently do any tag verification for any packets, but we can implement that later.

jeremija added a commit that referenced this pull request May 10, 2022

Add sctp.Association.Abort(reason string) method

23f7b42

According to RFC 4960. Also see my comment at #176. Closes #182. I don't think we currently do any tag verification for any packets, but we can implement that later.

jeremija added a commit that referenced this pull request May 10, 2022

Add sctp.Association.Abort(reason string) method

50c75e6

According to RFC 4960. Also see my comment at #176. Closes #182. I don't think we currently do any tag verification for any packets, but we can implement that later.

enobufs pushed a commit that referenced this pull request May 11, 2022

Add sctp.Association.Abort(reason string) method

e4e606d

According to RFC 4960. Also see my comment at #176. Closes #182. I don't think we currently do any tag verification for any packets, but we can implement that later.

stv0g deleted the jeremija/issue-152-shutdown-seq branch March 1, 2024 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement shutdown sequence #176

Implement shutdown sequence #176

jeremija commented Dec 20, 2020 •

edited

Loading

codecov bot commented Dec 20, 2020 •

edited

Loading

Sean-Der commented Dec 25, 2020

enobufs commented Feb 13, 2021

enobufs left a comment •

edited

Loading

enobufs Feb 14, 2021 •

edited

Loading

jeremija Feb 17, 2021 •

edited

Loading

enobufs Feb 17, 2021 •

edited

Loading

Sean-Der Feb 18, 2021

jeremija Feb 18, 2021

jeremija Feb 18, 2021 •

edited

Loading

jeremija commented Feb 17, 2021 •

edited

Loading

enobufs left a comment •

edited

Loading

jeremija commented Feb 20, 2021 •

edited

Loading

Implement shutdown sequence #176

Implement shutdown sequence #176

Conversation

jeremija commented Dec 20, 2020 • edited Loading

Description

Reference issue

codecov bot commented Dec 20, 2020 • edited Loading

Codecov Report

Sean-Der commented Dec 25, 2020

enobufs commented Feb 13, 2021

enobufs left a comment • edited Loading

Choose a reason for hiding this comment

enobufs Feb 14, 2021 • edited Loading

Choose a reason for hiding this comment

jeremija Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

enobufs Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Sean-Der Feb 18, 2021

Choose a reason for hiding this comment

jeremija Feb 18, 2021

Choose a reason for hiding this comment

jeremija Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

jeremija commented Feb 17, 2021 • edited Loading

enobufs left a comment • edited Loading

Choose a reason for hiding this comment

jeremija commented Feb 20, 2021 • edited Loading

jeremija commented Dec 20, 2020 •

edited

Loading

codecov bot commented Dec 20, 2020 •

edited

Loading

enobufs left a comment •

edited

Loading

enobufs Feb 14, 2021 •

edited

Loading

jeremija Feb 17, 2021 •

edited

Loading

enobufs Feb 17, 2021 •

edited

Loading

jeremija Feb 18, 2021 •

edited

Loading

jeremija commented Feb 17, 2021 •

edited

Loading

enobufs left a comment •

edited

Loading

jeremija commented Feb 20, 2021 •

edited

Loading