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

Fix Association and Stream closure #236

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mafredri
Copy link

@mafredri mafredri commented Aug 2, 2022

Description

  • Always close Association on writeLoop exit

The connection will now always be closed on writeLoop exit because it
will ensure that readLoop exits, which is needed to propagate the
closing of Streams.

  • Guard against creating Streams after Association close

It was possible for new Streams to be created after readLoop has
exited and called unregisterStream on the existing ones. The new
Streams would never close.

This also guards against a potential panic due to send on nil channel
(acceptCh).

Reference issue

Fixes pion/webrtc#2098 (with high probability)

Notes

Included a few fixes to tests to improve stability when run with go test -race.

@Sean-Der
Copy link
Member

Sean-Der commented Aug 2, 2022

Really great job @mafredri ! Would you mind squashing these into one commit?

I will add you to the Pion org so your tests will always run. @enobufs does this match with the patterns already?

@codecov
Copy link

codecov bot commented Aug 2, 2022

Codecov Report

Merging #236 (a0a2c67) into master (d0b7cf3) will increase coverage by 0.00%.
The diff coverage is 73.33%.

❗ Current head a0a2c67 differs from pull request most recent head e4d7a2f. Consider uploading reports for the commit e4d7a2f to get more accurate results

@@           Coverage Diff           @@
##           master     #236   +/-   ##
=======================================
  Coverage   80.86%   80.87%           
=======================================
  Files          48       48           
  Lines        3962     3968    +6     
=======================================
+ Hits         3204     3209    +5     
- Misses        619      620    +1     
  Partials      139      139           
Flag Coverage Δ
go 80.87% <73.33%> (+<0.01%) ⬆️
wasm 66.75% <46.66%> (-0.08%) ⬇️

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

Impacted Files Coverage Δ
association.go 84.68% <73.33%> (-0.01%) ⬇️

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 d0b7cf3...e4d7a2f. Read the comment docs.

* Always close `Association` on `writeLoop` exit

The connection will now always be closed on `writeLoop` exit because it
will ensure that `readLoop` exits, which is needed to propagate the
closing of `Stream`s.

* Guard against creating `Stream`s after `Association` close

It was possible for new `Stream`s to be created after `readLoop` has
exited and called `unregisterStream` on the existing ones. The new
`Stream`s would never close.

This also guards against a potential panic due to send on nil channel
(`acceptCh`).

This may fix pion/webrtc#2098.
@mafredri mafredri force-pushed the mafredri/association-stream-closure branch from a0a2c67 to e4d7a2f Compare August 2, 2022 19:22
@mafredri
Copy link
Author

mafredri commented Aug 2, 2022

@Sean-Der sure thing, done!

@mafredri mafredri changed the title Fix Association and Stream closure Fix Association and Stream closure Aug 2, 2022
@forcom
Copy link

forcom commented Sep 27, 2022

This PR breaks some tests in pion/webrtc.
The error message in the broken tests is datachannel_test.go:26: Failed to close PeerConnection: dtls fatal: conn is closed.
In the situation that the association is disconnected without calling Close(), when (*Association).Close() is called explicitly, the double closure of netConn emits such error.

I suggest that make (*Association).close() be called only once like below.

func (a *Association) close() error {
	a.log.Debugf("[%s] closing association..", a.name)

	a.closeOnce.Do(func() {
		a.setState(closed)

		err := a.netConn.Close()
		if err != nil {
			a.closeErr.Store(err)
		}

		a.closeAllTimers()

		// awake writeLoop to exit
		a.closeWriteLoopOnce.Do(func() { close(a.closeWriteLoopCh) })
	})

	err := a.closeErr.Load()
	if err != nil {
		return err.(error)
	}
	return nil
}

@edaniels
Copy link
Member

hi @mafredri, are you still interested in pushing this PR forward?

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.

DataChannel.readLoop goroutine leak
4 participants