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

Broadcaster: Don't pass a nil context into grpc call or it panics #2586

Merged
merged 9 commits into from
Sep 16, 2022

Conversation

thomshutt
Copy link
Contributor

@thomshutt thomshutt commented Sep 13, 2022

What does this pull request do? Explain your changes. (required)
Stops a panic in the case we detect an existing session after creating one

Specific updates (required)

  • Pass context.Background() instead of nil into the gRPC call

How did you test each of these updates (required)

  • Ran unit tests

Does this pull request close any open issues?
Fixes #2585

Checklist:

@thomshutt thomshutt changed the title Don't pass a nil context into grpc call or it panics Broadcaster: Don't pass a nil context into grpc call or it panics Sep 13, 2022
@thomshutt thomshutt marked this pull request as ready for review September 13, 2022 07:47
@codecov
Copy link

codecov bot commented Sep 13, 2022

Codecov Report

Merging #2586 (47f5e9c) into master (6bf2087) will increase coverage by 0.00269%.
The diff coverage is 73.13433%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #2586         +/-   ##
===================================================
+ Coverage   56.42997%   56.43266%   +0.00269%     
===================================================
  Files             88          88                 
  Lines          18857       18888         +31     
===================================================
+ Hits           10641       10659         +18     
- Misses          7637        7650         +13     
  Partials         579         579                 
Impacted Files Coverage Δ
eth/client.go 5.16039% <0.00000%> (-0.11779%) ⬇️
server/mediaserver.go 67.66744% <96.07843%> (+0.84501%) ⬆️
discovery/discovery.go 89.16667% <0.00000%> (-2.50000%) ⬇️

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 3ff79bd...47f5e9c. Read the comment docs.

Impacted Files Coverage Δ
eth/client.go 5.16039% <0.00000%> (-0.11779%) ⬇️
server/mediaserver.go 67.66744% <96.07843%> (+0.84501%) ⬆️
discovery/discovery.go 89.16667% <0.00000%> (-2.50000%) ⬇️

server/mediaserver.go Outdated Show resolved Hide resolved
@cyberj0g
Copy link
Contributor

cyberj0g commented Sep 14, 2022

It seems that explicitly marking connection as being initialized will allow to remove that rarely invoked and not covered 'rollback' code path. See my changes.

Edit: looks like there were a number of race conditions in registerConnection and elsewhere, we indirectly encountered only one of them. Hope it's better with this fix.

@cyberj0g cyberj0g marked this pull request as draft September 14, 2022 10:54
@cyberj0g cyberj0g marked this pull request as ready for review September 14, 2022 16:17
server/mediaserver.go Outdated Show resolved Hide resolved
server/mediaserver.go Outdated Show resolved Hide resolved
server/mediaserver.go Outdated Show resolved Hide resolved
@cyberj0g
Copy link
Contributor

Thanks for review @yondonfu, all addressed.

Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

LGTM. The refactored connection handling code seems cleaner and easier to reason about 👍 .

Before merging please update CHANGELOG_PENDING to reflect that this PR resolves the linked issue in a different way than the original PR title/description.

@cyberj0g cyberj0g merged commit 5d4e145 into master Sep 16, 2022
yondonfu added a commit that referenced this pull request Sep 18, 2022
yondonfu added a commit that referenced this pull request Sep 18, 2022
@hjpotter92 hjpotter92 deleted the fix-nil-context-cleanup branch September 26, 2022 17:00
@cyberj0g cyberj0g mentioned this pull request Oct 21, 2022
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.

panic in EndTranscodingSession
3 participants