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.

Sign up for GitHub

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

Jump to bottom

DataChannel.readLoop goroutine leak #2098

Open
lenaky opened this issue Jan 24, 2022 · 3 comments · May be fixed by pion/sctp#236
Open

DataChannel.readLoop goroutine leak #2098

lenaky opened this issue Jan 24, 2022 · 3 comments · May be fixed by pion/sctp#236

Comments

@lenaky
Copy link
Contributor

lenaky commented Jan 24, 2022 •
edited

Your environment.

  • Version: Release or SHA
  • Browser: include version
  • Other Information - stacktraces, related issues, suggestions how to fix, links for us to have context

What did you do?

I got goroutine leak from our system which is based on ion-sfu.

(dlv) bt
0  0x000000000043cc65 in runtime.gopark
   at /__w/_tool/go/1.16.4/x64/src/runtime/proc.go:337
1  0x000000000046fe38 in runtime.goparkunlock
   at /__w/_tool/go/1.16.4/x64/src/runtime/proc.go:342
2  0x000000000046fe38 in sync.runtime_notifyListWait
   at /__w/_tool/go/1.16.4/x64/src/runtime/sema.go:513
3  0x000000000047b879 in sync.(*Cond).Wait
   at /__w/_tool/go/1.16.4/x64/src/sync/cond.go:56
4  0x000000000071fd56 in github.com/pion/sctp.(*Stream).ReadSCTP
   at /github/home/go/pkg/mod/github.com/pion/sctp@v1.8.2/stream.go:108
5  0x00000000007271bd in github.com/pion/datachannel.(*DataChannel).ReadDataChannel
   at /github/home/go/pkg/mod/github.com/pion/datachannel@v1.5.2/datachannel.go:186
6  0x00000000007ee3da in git.dev.hpcnt.com/hyperconnect/pion-webrtc/v3.(*DataChannel).readLoop
   at /github/home/go/pkg/mod/git.dev.hpcnt.com/hyperconnect/pion-webrtc/v3@v3.1.2-hpcnt.release/datachannel.go:324

As you can see, Stream.ReadSCTP hangs while waiting for signal at Stream.readNotifier.
at that moment association and dataChannel have been already closed.

(dlv) p s.association.readLoopCloseCh
chan struct {} {
	qcount: 0,
	dataqsiz: 0,
	buf: *[0]struct struct {} [],
	elemsize: 0,
	closed: 1,
	elemtype: *runtime._type {size: 0, ptrdata: 0, hash: 670477339, tflag: tflagExtraStar|tflagRegularMemory (10), align: 1, fieldAlign: 1, kind: 25, equal: runtime.memequal0, gcdata: *0, str: 56587, ptrToThis: 543648},
	sendx: 0,
	recvx: 0,
	recvq: waitq<struct {}> {
		first: *sudog<struct {}> nil,
		last: *sudog<struct {}> nil,},
	sendq: waitq<struct {}> {
		first: *sudog<struct {}> nil,
		last: *sudog<struct {}> nil,},
	lock: runtime.mutex {
		lockRankStruct: runtime.lockRankStruct {},
		key: 0,},}
(dlv) p s.association.closeWriteLoopCh
chan struct {} {
	qcount: 0,
	dataqsiz: 0,
	buf: *[0]struct struct {} [],
	elemsize: 0,
	closed: 1,
	elemtype: *runtime._type {size: 0, ptrdata: 0, hash: 670477339, tflag: tflagExtraStar|tflagRegularMemory (10), align: 1, fieldAlign: 1, kind: 25, equal: runtime.memequal0, gcdata: *0, str: 56587, ptrToThis: 543648},
	sendx: 0,
	recvx: 0,
	recvq: waitq<struct {}> {
		first: *sudog<struct {}> nil,
		last: *sudog<struct {}> nil,},
	sendq: waitq<struct {}> {
		first: *sudog<struct {}> nil,
		last: *sudog<struct {}> nil,},
	lock: runtime.mutex {
		lockRankStruct: runtime.lockRankStruct {},
		key: 0,},}
(dlv) p s.association.streams
map[uint16]*github.com/pion/sctp.Stream [
	0: *{
		association: *(*"github.com/pion/sctp.Association")(0xc0431f9500),
		lock: (*sync.RWMutex)(0xc01a279e08),
		streamIdentifier: 0,
		defaultPayloadType: PayloadTypeWebRTCBinary (53),
		reassemblyQueue: *(*"github.com/pion/sctp.reassemblyQueue")(0xc072a4a840),
		sequenceNumber: 1,
		readNotifier: *(*sync.Cond)(0xc04c6bea80),
		readErr: error nil,
		writeErr: error nil,
		unordered: false,
		reliabilityType: 0,
		reliabilityValue: 0,
		bufferedAmount: 19,
		bufferedAmountLow: 0,
		onBufferedAmountLow: nil,
		log: github.com/pion/logging.LeveledLogger(*github.com/pion/logging.DefaultLeveledLogger) ...,
		name: "0:0xc0431f9500",},
]

I presume while SCTPTransport.Start is in progress, Association.readLoop is closed and
Association.unregisterStream which is defer action does not affect if Association.OpenStream not called yet.

I don't know exactly what causes this. I think may be It happens when peerConnection closed while data channel connection is being established.

What did you expect?

What happened?
@Sean-Der Sean-Der added this to the 3.2.0 milestone Feb 5, 2022
This was referenced Mar 31, 2022
Closed
Closed
@Sean-Der Sean-Der mentioned this issue May 22, 2022
Closed
@Sean-Der Sean-Der removed this from the 3.2.0 milestone May 22, 2022
@mafredri mafredri linked a pull request Aug 2, 2022 that will close this issue
Open
mafredri added a commit to mafredri/sctp that referenced this issue Aug 2, 2022
@mafredri
Guard against creating Streams after Association close
e445d87 
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 may fix pion/webrtc#2098.
@mafredri
Copy link

mafredri commented Aug 2, 2022

@lenaky Does pion/sctp#236 solve your issue?

mafredri added a commit to mafredri/sctp that referenced this issue Aug 2, 2022
@mafredri
Fix Association and Stream closure
e4d7a2f 
* 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.
@lenaky
Copy link
Contributor Author

lenaky commented Aug 31, 2022 •
edited

@mafredri sorry for late. I didn't fix this issue and I'll check and let u know ur PR work for me. thanks!

@forcom
Copy link
Contributor

forcom commented Sep 27, 2022

@mafredri On behalf of @lenaky, I apply your PR to check this issue.
It seems that the issue does not reappear with some modification which I left in yours.
Thanks for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@mafredri @forcom @Sean-Der @lenaky