server: B should not retry transcoding if client closed/canceled the connection #2124
Conversation
server/broadcast_test.go
Outdated
| assert.NotNil(err) | ||
| assert.Equal("Hit max transcode attempts: UnknownResponse", err.Error()) | ||
| assert.Contains("context canceled", err.Error()) | ||
| assert.Equal(2, transcodeCalls, "Segment submission calls did not match") |
There was a problem hiding this comment.
Shouldn't this be 1?
Better yet, should we check if the context is cancelled before we make the attempt, so that we don't try even once in case the context is cancelled before we start trying?
There was a problem hiding this comment.
No, it shouldn't be one. This value is aggregated across the whole test. Meaning:
+1in line 781+1in line 791 (because we try one time to transcode)
Is it good or bad that we have one unit test for basically one function, not a separate function for one test scenario, it's a good question. I'd personally change it to have one unit test function per each scenario, but here I want to be consistent with what we do in the whole project.
Better yet, should we check if the context is cancelled before we make the attempt, so that we don't try even once in case the context is cancelled before we start trying?
It boils down to what we discussed #2124, especially these comments:
It would make sense to me to always make a "reasonable attempt" to complete the transcode if a segment was (fully?) received, regardless of client connection status.
And
it doesn't completely break the ability to use ffmpeg to push segments and then query for a playlist after
The thing is that if we don't try to transcode even once, then If the connection is suddenly closed, we don't have any results.
There was a problem hiding this comment.
The thing is that if we don't try to transcode even once, then If the connection is suddenly closed, we don't have any results.
Makes total sense, agreed!
No, it shouldn't be one. This value is aggregated across the whole test. Meaning:
I see... Yeah, this running of multiple test cases on the same test is quite confusing right now, so IMO it's bad. We could theorically create some setup functions and call them before each case, but there are some nice test libraries out there that we could use which make this much easier.
One of those that I really like (but also one of the few that I have XP with) is goconvey which has some nice sugar for setting things up so that you write the setup code on the same function where you're going to use it, but it's also re-executed for each "leaf" test function (parent functions basically work as "before each" steps). An example of where I've used it: https://github.com/victorges/nuledger/blob/main/authorizer/handler_test.go#L68
As for our current case, WDYT of resetting the transcodeCalls on the beginning of each test case, so that we can deal with a more reasonable number in each of these cases? Could be done in the same line where we set the MaxAttempts like MaxAttempts, transcodeCalls = 10, 0
There was a problem hiding this comment.
Either way, total optional/nitpick change now that I understood this is correct. Feel free to just resolve the comment and leave this as is.
There was a problem hiding this comment.
One of those that I really like (but also one of the few that I have XP with) is goconvey which has some nice sugar for setting things up so that you write the setup code on the same function where you're going to use it, but it's also re-executed for each "leaf" test function (parent functions basically work as "before each" steps). An example of where I've used it: https://github.com/victorges/nuledger/blob/main/authorizer/handler_test.go#L68
Yeah, In my opinion, it'd also be better. Now these tests are messy.
As for our current case, WDYT of resetting the transcodeCalls on the beginning of each test case, so that we can deal with a more reasonable number in each of these cases? Could be done in the same line where we set the MaxAttempts like MaxAttempts, transcodeCalls = 10, 0
Yeah, good idea. Updated.
Co-authored-by: Victor Elias <victorgelias@gmail.com>
server/broadcast_test.go
Outdated
| assert.NotNil(err) | ||
| assert.Equal("Hit max transcode attempts: UnknownResponse", err.Error()) | ||
| assert.Contains("context canceled", err.Error()) | ||
| assert.Equal(2, transcodeCalls, "Segment submission calls did not match") |
There was a problem hiding this comment.
No, it shouldn't be one. This value is aggregated across the whole test. Meaning:
+1in line 781+1in line 791 (because we try one time to transcode)
Is it good or bad that we have one unit test for basically one function, not a separate function for one test scenario, it's a good question. I'd personally change it to have one unit test function per each scenario, but here I want to be consistent with what we do in the whole project.
Better yet, should we check if the context is cancelled before we make the attempt, so that we don't try even once in case the context is cancelled before we start trying?
It boils down to what we discussed #2124, especially these comments:
It would make sense to me to always make a "reasonable attempt" to complete the transcode if a segment was (fully?) received, regardless of client connection status.
And
it doesn't completely break the ability to use ffmpeg to push segments and then query for a playlist after
The thing is that if we don't try to transcode even once, then If the connection is suddenly closed, we don't have any results.
What does this pull request do? Explain your changes. (required)
B will take any more attempts of sending the segment to O if the client closed/canceled the request connection.
Before, even if the client was not interested in transcoding anymore, B tried transcoding until the
maxAttempsnumber is reached.Specific updates (required)
How did you test each of these updates (required)
Introduced
printlnand sent requests from the script for two options:Does this pull request close any open issues?
fix #2113
Checklist:
makeruns successfully./test.shpass