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

Call session end async to avoid unnecessary blocking #2764

Merged
merged 3 commits into from
Mar 9, 2023
Merged

Call session end async to avoid unnecessary blocking #2764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e086ecd
Call session end async to avoid unnecessary blocking
mjh1 Mar 2, 2023
1b31f5d
Fix tests - we need to wait for the session end goroutines to finish
mjh1 Mar 6, 2023
f85d5c2
Fix issue with ffmpeg caching on CI
mjh1 Mar 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ jobs:
uses: actions/cache@v3
with:
path: /github/home/compiled
key: ${{ runner.os }}-ffmpeg-${{ hashFiles('**/install_ffmpeg.sh') }}
key: ${{ runner.os }}-ffmpeg-${{ hashFiles('install_ffmpeg.sh') }}
Copy link
Member

Choose a reason for hiding this comment

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

What was the reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Builds were failing on master like this: https://github.com/livepeer/go-livepeer/actions/runs/4309780285/jobs/7517555365
It seemed as though the wildcard matching was not working. @hjpotter92 made some changes in this area recently so I'll check with him when he's back but the change I've made has fixed the issue in any case.

restore-keys: |
${{ runner.os }}-ffmpeg

Expand Down
1 change: 1 addition & 0 deletions CHANGELOG_PENDING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

### Bug Fixes 🐞
- \# 2759 Parse keystore address without 0x prefix, fix parse error logging
- \# 2764 Call session end asynchronously to avoid unnecessary blocking

#### CLI

Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ require (
github.com/tyler-smith/go-bip39 v1.0.2 // indirect
github.com/urfave/cli v1.22.12
go.opencensus.io v0.24.0
go.uber.org/goleak v1.2.0
go.uber.org/goleak v1.2.2-0.20230213210001-751da596f6f7
golang.org/x/crypto v0.0.0-20211215153901-e495a2d5b3d3 // indirect
golang.org/x/net v0.0.0-20220722155237-a158d28d115b
google.golang.org/grpc v1.51.0
Expand Down
7 changes: 2 additions & 5 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1057,8 +1057,8 @@ go.opentelemetry.io/proto/otlp v0.7.0/go.mod h1:PqfVotwruBrMGOCsRd/89rSnXhoiJIqe
go.uber.org/atomic v1.3.2/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE=
go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE=
go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=
go.uber.org/goleak v1.2.0 h1:xqgm/S+aQvhWFTtR0XK3Jvg7z8kGV8P4X14IzwN3Eqk=
go.uber.org/goleak v1.2.0/go.mod h1:XJYK+MuIchqpmGmUSAzotztawfKvYLUIgg7guXrwVUo=
go.uber.org/goleak v1.2.2-0.20230213210001-751da596f6f7 h1:cnmZW/hz/FJAWBKyZlR57sJRY5htWrQbrykbUbJSbeI=
go.uber.org/goleak v1.2.2-0.20230213210001-751da596f6f7/go.mod h1:qlT2yGI9QafXHhZZLxlSuNsMw3FFLxBr+tBRlmO1xH4=
go.uber.org/multierr v1.1.0/go.mod h1:wR5kodmAFQ0UK8QlbwjlSNy0Z68gJhDJUG5sjR94q/0=
go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU=
go.uber.org/zap v1.9.1/go.mod h1:vwi/ZaCAaUcBkycHslxD9B2zi4UTXhF60s6SWpuDF0Q=
Expand Down Expand Up @@ -1113,7 +1113,6 @@ golang.org/x/lint v0.0.0-20191125180803-fdd1cda4f05f/go.mod h1:5qLYkcX4OjUUV8bRu
golang.org/x/lint v0.0.0-20200130185559-910be7a94367/go.mod h1:3xt1FjdF8hUf6vQPIChWIBhFzV8gjjsPE/fR3IyQdNY=
golang.org/x/lint v0.0.0-20200302205851-738671d3881b/go.mod h1:3xt1FjdF8hUf6vQPIChWIBhFzV8gjjsPE/fR3IyQdNY=
golang.org/x/lint v0.0.0-20201208152925-83fdc39ff7b5/go.mod h1:3xt1FjdF8hUf6vQPIChWIBhFzV8gjjsPE/fR3IyQdNY=
golang.org/x/lint v0.0.0-20210508222113-6edffad5e616 h1:VLliZ0d+/avPrXXH+OakdXhpJuEoBZuwh1m2j7U6Iug=
golang.org/x/lint v0.0.0-20210508222113-6edffad5e616/go.mod h1:3xt1FjdF8hUf6vQPIChWIBhFzV8gjjsPE/fR3IyQdNY=
golang.org/x/mobile v0.0.0-20190312151609-d3739f865fa6/go.mod h1:z+o9i4GpDbdi3rU15maQ/Ox0txvL9dWGYEHz965HBQE=
golang.org/x/mobile v0.0.0-20190719004257-d2bd2a29d028/go.mod h1:E/iHnbuqvinMTCcRqshq8CkpyQDoeVncDDYHnLhea+o=
Expand All @@ -1128,7 +1127,6 @@ golang.org/x/mod v0.4.1/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
golang.org/x/mod v0.4.2/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
golang.org/x/mod v0.5.1/go.mod h1:5OXOZSfqPIIbmVBIIKWRFfZjPR0E5r58TLhUjH0a2Ro=
golang.org/x/mod v0.6.0-dev.0.20211013180041-c96bc1413d57/go.mod h1:3p9vT2HGsQu2K1YbXdKPJLVgG5VJdoTa1poYQBtP1AY=
golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4 h1:6zppjxzCulZykYSLyVDYbneBfbaBIQPYMevg0bEwv2s=
golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4=
golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
Expand Down Expand Up @@ -1444,7 +1442,6 @@ golang.org/x/tools v0.1.3/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk=
golang.org/x/tools v0.1.4/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk=
golang.org/x/tools v0.1.5/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk=
golang.org/x/tools v0.1.8-0.20211029000441-d6a9af8af023/go.mod h1:nABZi5QlRsZVlzPpHl034qft6wpY4eDcsTt5AaioBiU=
golang.org/x/tools v0.1.12 h1:VveCTK38A2rkS8ZqFY25HIDFscX5X9OoEhJd3quQmXU=
golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc=
golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
Expand Down
8 changes: 5 additions & 3 deletions server/broadcast.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,9 +682,11 @@ func (bsm *BroadcastSessionsManager) collectResults(submitResultsCh chan *Submit
// the caller needs to ensure bsm.sessLock is acquired before calling this.
func (bsm *BroadcastSessionsManager) completeSessionUnsafe(ctx context.Context, sess *BroadcastSession, tearDown bool) {
if tearDown {
if err := EndTranscodingSession(ctx, sess); err != nil {
clog.Errorf(ctx, "Error completing transcoding session: %q", err)
}
go func() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yondonfu @iameli Are you guys happy with this? I've been looking at the B code downstream of here as well as the function that ends up being called on the O side and can't see a need for blocking.

Copy link
Member

Choose a reason for hiding this comment

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

AFAICT you seem to be right that this call doesn't need to block for B.

if err := EndTranscodingSession(ctx, sess); err != nil {
clog.Errorf(ctx, "Error completing transcoding session: %q", err)
}
}()
}
if sess.OrchestratorScore == common.Score_Untrusted {
bsm.untrustedPool.completeSession(sess)
Expand Down
9 changes: 5 additions & 4 deletions server/mediaserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,13 @@ import (
ffmpeg "github.com/livepeer/lpms/ffmpeg"
"github.com/livepeer/lpms/segmenter"
"github.com/livepeer/lpms/stream"
"go.uber.org/goleak"
)

// var S *LivepeerServer

var pushResetWg sync.WaitGroup // needed to synchronize exits from HTTP push

// func setupServer() *LivepeerServer {
// s, _ := setupServerWithCancel()
// return s
// }
var port = 10000

// waitForTCP tries to establish TCP connection for a specified time
Expand Down Expand Up @@ -1226,6 +1223,10 @@ func TestRegisterConnection(t *testing.T) {
}

func TestBroadcastSessionManagerWithStreamStartStop(t *testing.T) {
goleakOptions := common.IgnoreRoutines()
// allow enough time for the transcode end goroutines to finish
goleakOptions = append(goleakOptions, goleak.MaxSleepInterval(5*time.Second), goleak.MaxRetryAttempts(1000))
defer goleak.VerifyNone(t, goleakOptions...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 1000 retry attempts and 5 seconds delay (I believe it's a delay between attempts, not overall)? I think, the EndTranscodingSession goroutine will finish instantly here. Also maybe pass options to VerifyNone call directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @cyberj0g. Unfortunately for the tests the communication for the EndTranscodingSession call isn't set up so it simply times out after the 3 second timeout, so that's the reason for waiting. And the other annoying thing is the goleak library starts off with very small microsecond pauses between retries so that's the reason for the high number.
I've looked into a lot of different ways of abstracting this away for the tests without success, I think it'll have to wait for a later PR.

assert := assert.New(t)

s, cancel := setupServerWithCancel()
Expand Down