Make connection retry timeout default consistent#105
Make connection retry timeout default consistent#105JetForMe wants to merge 2 commits intoswift-server:mainfrom
Conversation
Makes the connection retry timeout consistent regardless of whether no value is specified, or `nil` is specified.
Joannis
left a comment
Joannis
left a comment
There was a problem hiding this comment.
LGTM. @fabianfett how can I trigger CI with the current setup?
|
@swift-server-bot please test |
|
@swift-server-bot test |
1 similar comment
|
@swift-server-bot test |
|
@swift-server-bot please test |
|
@swift-server-bot add to allowlist |
|
I'm not sure what the tests are trying to do here, but I notice that only "soundness," 5.6, 5.7, and 5.8 have a "required" label, but the rest do not (in particular, 5.9 and 5.10). |
|
Welp, my code no longer builds. I'm not sure why. Let me fix it. |
|
@0xTim Sorry about that, not sure what happened. But all’s good now, I think. |
|
@Joannis I think this is good to go? |
| self.connectionRetryConfiguration = ( | ||
| (initialConnectionBackoffDelay, connectionBackoffFactor), | ||
| connectionRetryTimeout ?? .milliseconds(10) // always default to a baseline 10ms | ||
| connectionRetryTimeout ?? Self.defaultConnectionRetryTimeout |
There was a problem hiding this comment.
this is a breaking change. Explicitly passing nil currently leads to a timeout of 10ms. We would change that to 60sec. This can lead to unexpected behavior for adopters.
There was a problem hiding this comment.
Sorry for taking so long to review this. Thanks for raising the issue and providing a potential fix.
While I agree that #104 absolutely has a point, I think that this change is a breaking behavior change, that we should not go forward with today. This issue can only be fixed in the next major. Adopters have a clear way around this issue, by passing an explicit value or omitting the parameter. Just explicitly passing nil is currently a bad option.
|
While I am generally loathe to make breaking changes, I’m not sure in this case it’s cut-and-dry. Here are my thoughts.
At the very least, the documentation and example should make it clear this is an issue. |
|
It's also not a breaking change - it's a change in behaviour for sure but code will still compile which is what SemVer defines as breaking changes. Whether it should be accepted is still a question for the maintainers though |
4 participants
Fix for #104. Makes the connection retry timeout consistent regardless of whether no value is specified, or
nilis specified.Test failures seem to be due to the specific test comparing floats:
Test Suite 'StringCommandsTests' failed at 2024-03-20 15:05:53.225.
Executed 17 tests, with 2 failures (0 unexpected) in 0.576 (0.577) seconds
Test Suite 'RediStackPackageTests.xctest' failed at 2024-03-20 15:05:53.225.
Executed 264 tests, with 2 failures (0 unexpected) in 38.703 (38.716) seconds
Test Suite 'All tests' failed at 2024-03-20 15:05:53.225.
Executed 264 tests, with 2 failures (0 unexpected) in 38.703 (38.717) seconds