Fix segfault in setting a pricePerBroadcaster for 3 or more B's #2820

stronk-dev · 2023-07-01T10:14:41Z

Fixes #2819
Maybe we want to add some more checks or logging of what the error is
cc @eliteprox

leszko · 2023-07-03T07:01:15Z

@stronk-dev could you fix go fmt, so that CI build would not fail?

thomshutt · 2023-07-03T08:15:30Z

common/readfromfile.go

@@ -11,6 +11,9 @@ import (
 // A valid string will always be returned, regardless of whether an error occurred.
 func ReadFromFile(s string) (string, error) {
 	info, err := os.Stat(s)
+	if err != nil {
+		return s, err
+	}
 	if os.IsNotExist(err) {


Doesn't this check now become redundant?

true, removed it

stronk-dev · 2023-07-03T10:43:05Z

Seems like one of the unit tests is failing now, as the ReadFromFile function used to return a nil error for non-existing files/directories. It now returns a PathError instead.
Should I modify the unit test to expect a PathError?

thomshutt · 2023-07-03T13:29:07Z

Should I modify the unit test to expect a PathError?

Yeah, I think that makes sense

thomshutt · 2023-07-05T07:54:49Z

@stronk-dev I don't think it's caused by this PR but if you can figure out what's going on with the tests then that'd be great

stronk-dev · 2023-07-05T08:39:38Z

It seems to fail in the TestHTTPPushBroadcaster where it tries to send segments to the broadcaster, but it returns a 503 error code. There are some errors before it fails the test:

E0704 13:23:40.096942   17647 roundinitializer.go:90] ErrRoundInitialized
E0704 13:23:53.983949   17647 db_discovery.go:331] Could not get orchestrator orch=[https://127.0.0.1:8938:](https://127.0.0.1:8938/) rpc error: code = Unknown desc = insufficient sender reserve
E0704 13:23:57.133644   17647 mediaserver.go:1033] manifestID=71abd7357f1325b54da0 nonce=17646280690537945479 seqNo=0 clientIP=127.0.0.1 No sessions available name=0.ts url=http://127.0.0.1:8939/live/71abd7357f1325b54da0/0.ts
E0704 13:23:57.148748   17647 gaspricemonitor.go:122] error getting gas price: Post "http://172.17.0.1:32774/": EOF
E0704 13:23:57.366899   17647 webserver.go:22] http: Server closed
E0704 13:23:57.367317   17647 webserver.go:22] http: Server closed

For the rest I'm not too familiar with the code or how these tests are ran to be able to fix it. From where the modified ReadFromFile function gets called, the returned error value isn't even used so not sure how these tests suddenly fail now

codecov · 2023-09-05T12:18:28Z

Codecov Report

Merging #2820 (e0bc83e) into master (a231510) will decrease coverage by 0.19014%.
Report is 6 commits behind head on master.
The diff coverage is 8.49057%.

@@                 Coverage Diff                 @@
##              master       #2820         +/-   ##
===================================================
- Coverage   56.70668%   56.51654%   -0.19014%     
===================================================
  Files             88          88                 
  Lines          19160       19228         +68     
===================================================
+ Hits           10865       10867          +2     
- Misses          7699        7762         +63     
- Partials         596         599          +3

Files Changed	Coverage Δ
clog/clog.go	`59.87261% <ø> (ø)`
cmd/livepeer/starter/starter.go	`4.22907% <0.00000%> (-0.03381%)`	⬇️
cmd/livepeer_cli/livepeer_cli.go	`0.00000% <ø> (ø)`
cmd/livepeer_cli/wizard_bond.go	`0.00000% <0.00000%> (ø)`
cmd/livepeer_cli/wizard_rounds.go	`0.00000% <0.00000%> (ø)`
cmd/livepeer_cli/wizard_stats.go	`0.00000% <0.00000%> (ø)`
cmd/livepeer_cli/wizard_transcoder.go	`0.00000% <0.00000%> (ø)`
core/livepeernode.go	`60.00000% <0.00000%> (-18.00000%)`	⬇️
server/handlers.go	`53.07018% <5.55556%> (-0.76226%)`	⬇️
core/orchestrator.go	`77.71664% <25.00000%> (-0.61669%)`	⬇️
... and 4 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f601912...e0bc83e. Read the comment docs.

Files Changed	Coverage Δ
clog/clog.go	`59.87261% <ø> (ø)`
cmd/livepeer/starter/starter.go	`4.22907% <0.00000%> (-0.03381%)`	⬇️
cmd/livepeer_cli/livepeer_cli.go	`0.00000% <ø> (ø)`
cmd/livepeer_cli/wizard_bond.go	`0.00000% <0.00000%> (ø)`
cmd/livepeer_cli/wizard_rounds.go	`0.00000% <0.00000%> (ø)`
cmd/livepeer_cli/wizard_stats.go	`0.00000% <0.00000%> (ø)`
cmd/livepeer_cli/wizard_transcoder.go	`0.00000% <0.00000%> (ø)`
core/livepeernode.go	`60.00000% <0.00000%> (-18.00000%)`	⬇️
server/handlers.go	`53.07018% <5.55556%> (-0.76226%)`	⬇️
core/orchestrator.go	`77.71664% <25.00000%> (-0.61669%)`	⬇️
... and 4 more

leszko · 2023-09-05T14:33:24Z

@thomshutt I think the CI passes now and I approved the PR, do you want to re-review or should I merge?

thomshutt · 2023-09-05T15:22:41Z

@leszko let's merge!

…peer#2820) * Fix segfault in setting a pricePerBroadcaster for 3 or more B's * go fmt * Remove error * Modify the `TestReadFromFileNoFileExists` unit test to expect an error * Fix typo --------- Co-authored-by: Thom Shutt <[email protected]>

Fix segfault in setting a pricePerBroadcaster for 3 or more B's

471c037

leszko approved these changes Jul 3, 2023

View reviewed changes

go fmt

bf79097

thomshutt reviewed Jul 3, 2023

View reviewed changes

Remove error

f27b333

Modify the TestReadFromFileNoFileExists unit test to expect an error

c64ce5a

thomshutt added 3 commits July 24, 2023 15:01

Merge branch 'master' into md/fixPricePerBroadcaster

7bc402a

Fix typo

e376f0b

Merge branch 'master' into md/fixPricePerBroadcaster

e0bc83e

leszko requested a review from thomshutt September 5, 2023 14:33

thomshutt merged commit 8bb2d49 into master Sep 5, 2023
17 checks passed

thomshutt deleted the md/fixPricePerBroadcaster branch September 5, 2023 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix segfault in setting a pricePerBroadcaster for 3 or more B's #2820

Fix segfault in setting a pricePerBroadcaster for 3 or more B's #2820

stronk-dev commented Jul 1, 2023

leszko commented Jul 3, 2023

thomshutt Jul 3, 2023

stronk-dev Jul 3, 2023

stronk-dev commented Jul 3, 2023

thomshutt commented Jul 3, 2023 •

edited

Loading

thomshutt commented Jul 5, 2023

stronk-dev commented Jul 5, 2023

codecov bot commented Sep 5, 2023

leszko commented Sep 5, 2023

thomshutt commented Sep 5, 2023

Fix segfault in setting a pricePerBroadcaster for 3 or more B's #2820

Fix segfault in setting a pricePerBroadcaster for 3 or more B's #2820

Conversation

stronk-dev commented Jul 1, 2023

leszko commented Jul 3, 2023

thomshutt Jul 3, 2023

Choose a reason for hiding this comment

stronk-dev Jul 3, 2023

Choose a reason for hiding this comment

stronk-dev commented Jul 3, 2023

thomshutt commented Jul 3, 2023 • edited Loading

thomshutt commented Jul 5, 2023

stronk-dev commented Jul 5, 2023

codecov bot commented Sep 5, 2023

Codecov Report

leszko commented Sep 5, 2023

thomshutt commented Sep 5, 2023

thomshutt commented Jul 3, 2023 •

edited

Loading