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

Unblock handshake ch on close #349

Merged
merged 1 commit into from
Aug 13, 2024
Merged

Unblock handshake ch on close #349

merged 1 commit into from
Aug 13, 2024

Conversation

edaniels
Copy link
Member

Fixes:

panic: test timed out after 10m0s
running tests:
	TestAssociation_createClientWithContext (9m30s)

goroutine 381 [running]:
testing.(*M).startAlarm.func1()
	/usr/local/go/src/testing/testing.go:2366 +0x38c
created by time.goFunc
	/usr/local/go/src/time/sleep.go:177 +0x2f

goroutine 1 [chan receive, 9 minutes]:
testing.(*T).Run(0x8d42008, {0x82b13b0, 0x27}, 0x82bc794)
	/usr/local/go/src/testing/testing.go:1750 +0x3d8
testing.runTests.func1(0x8d42008)
	/usr/local/go/src/testing/testing.go:2161 +0x45
testing.tRunner(0x8d42008, 0x8d21dfc)
	/usr/local/go/src/testing/testing.go:1689 +0x125
testing.runTests(0x8c10220, {0x845e660, 0x56, 0x56}, {0xc1a6b70d06f1a027, 0x8bb2d357fb, 0x8460720})
	/usr/local/go/src/testing/testing.go:2159 +0x39d
testing.(*M).Run(0x8c16320)
	/usr/local/go/src/testing/testing.go:2027 +0x6d8
main.main()
	_testmain.go:219 +0x141

goroutine 434 [select, 9 minutes]:
github.com/pion/sctp.TestAssociation_createClientWithContext(0x8d42108)
	/go/src/github.com/pion/sctp/association_test.go:3267 +0x293
testing.tRunner(0x8d42108, 0x82bc794)
	/usr/local/go/src/testing/testing.go:1689 +0x125
created by testing.(*T).Run in goroutine 1
	/usr/local/go/src/testing/testing.go:1742 +0x3b9

goroutine 435 [chan receive, 9 minutes]:
github.com/pion/sctp.(*Association).Close(0x9106008)
	/go/src/github.com/pion/sctp/association.go:508 +0xfc
github.com/pion/sctp.createClientWithContext({0x82fc480, 0x8c160f0}, {{0x0, 0x0}, {0x82fc8f4, 0x9030000}, 0x0, 0x0, 0x0, {0x82fb710, ...}, ...})
	/go/src/github.com/pion/sctp/association.go:296 +0x234
github.com/pion/sctp.TestAssociation_createClientWithContext.func1()
	/go/src/github.com/pion/sctp/association_test.go:3237 +0x89
created by github.com/pion/sctp.TestAssociation_createClientWithContext in goroutine 434
	/go/src/github.com/pion/sctp/association_test.go:3236 +0x153

goroutine 418 [chan send, 9 minutes]:
github.com/pion/sctp.(*Association).handleCookieEcho(0x9106008, 0x90962e0)
	/go/src/github.com/pion/sctp/association.go:1379 +0x229
github.com/pion/sctp.(*Association).handleChunk(0x9106008, 0x8fe4150, {0x82fc240, 0x90962e0})
	/go/src/github.com/pion/sctp/association.go:2542 +0x184
github.com/pion/sctp.(*Association).handleInbound(0x9106008, {0x8c72510, 0x30, 0x30})
	/go/src/github.com/pion/sctp/association.go:712 +0x2da
github.com/pion/sctp.(*Association).readLoop(0x9106008)
	/go/src/github.com/pion/sctp/association.go:607 +0x1f7
created by github.com/pion/sctp.(*Association).init in goroutine 435
	/go/src/github.com/pion/sctp/association.go:393 +0xab

goroutine 419 [sync.Mutex.Lock, 9 minutes]:
sync.runtime_SemacquireMutex(0x910601c, 0x0, 0x1)
	/usr/local/go/src/runtime/sema.go:77 +0x3f
sync.(*Mutex).lockSlow(0x9106018)
	/usr/local/go/src/sync/mutex.go:171 +0x247
sync.(*Mutex).Lock(0x9106018)
	/usr/local/go/src/sync/mutex.go:90 +0x4c
sync.(*RWMutex).Lock(0x9106018)
	/usr/local/go/src/sync/rwmutex.go:146 +0x23
github.com/pion/sctp.(*Association).gatherOutbound(0x9106008)
	/go/src/github.com/pion/sctp/association.go:962 +0x5c
github.com/pion/sctp.(*Association).writeLoop(0x9106008)
	/go/src/github.com/pion/sctp/association.go:622 +0x169
created by github.com/pion/sctp.(*Association).init in goroutine 435

@edaniels edaniels requested a review from sukunrt August 12, 2024 20:49
Copy link

codecov bot commented Aug 12, 2024

Codecov Report

Attention: Patch coverage is 58.33333% with 5 lines in your changes missing coverage. Please review.

Project coverage is 81.11%. Comparing base (18c4015) to head (d7a4bdf).

Files Patch % Lines
association.go 58.33% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #349      +/-   ##
==========================================
- Coverage   81.34%   81.11%   -0.23%     
==========================================
  Files          51       51              
  Lines        3339     3347       +8     
==========================================
- Hits         2716     2715       -1     
- Misses        479      485       +6     
- Partials      144      147       +3     
Flag Coverage Δ
go 81.11% <58.33%> (-0.23%) ⬇️
wasm 67.31% <41.66%> (-0.08%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

association.go Outdated
Comment on lines 2718 to 2721
select {
case a.handshakeCompletedCh <- ErrHandshakeCookieEcho:
case <-a.closeWriteLoopCh: // check the read/write sides
case <-a.readLoopCloseCh:
Copy link
Member

Choose a reason for hiding this comment

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

Should we factor this out into
func (a *Association) completeHandshake(err error) error

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'm not sure given that not all the selects contain the same cases. Although making them the same cases probably can't hurt?

Copy link
Member

Choose a reason for hiding this comment

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

I am fine either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given they are receives, I'll refactor it :)

@edaniels edaniels force-pushed the handshake_close_race branch from 194ba43 to d7a4bdf Compare August 13, 2024 13:59
@edaniels edaniels requested a review from sukunrt August 13, 2024 13:59
@edaniels
Copy link
Member Author

@sukunrt made that change, ty for the suggestion.

@edaniels edaniels merged commit fae4852 into master Aug 13, 2024
12 of 13 checks passed
@edaniels edaniels deleted the handshake_close_race branch August 13, 2024 15:07
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.

2 participants