Make the interval at which re-REGISTER requests are sent configurable #902
Conversation
|
This Pull Request changes the code to remove the use of registrar's response containing the actual expires time. The code now assumes that the expire time passed in (or the default) is the actual expire time, which is not always the case. The registrar has the ability to set the time lower than what was requested. Making this configuration option be a percentage of the actual expires (so 1-99) would cover this. On a related note, this change also allows the user to set a reRegisterInterval that will get them in trouble if they set it too high. The set interval should either be ignored or dismissed with an error message if this occurs. The percentage solution would also squash this. I recognize I've suggested the percentage solution twice, but I'm sure other options could also be valid. |
|
I see. Perhaps the configuration option could then also be the number of seconds by which the |
c9035a6 to
9255b12
Compare
|
I've reworked the code so that the value passed as an option represents a percentage of the actual expiration time |
|
This looks much better, one small change. We want this to be named refreshFrequency, this name aligns more with RFC 3261. We will get around to taking the PR as is and making the changes on top, but if you want to make these changes prior to that, that would be great. Thanks again! |
|
Just some disinterested party thoughts - 1% seems like a really low lower bound. 50% seems more reasonable to me. |
|
We also had that thought and shrugged it off, but you're right, we should keep the lane as safe as possible for the unknowing user out there. That's another change to get in if the PR isn't changed by the time we take it, thanks. |
9255b12 to
3e2d865
Compare
src/api/registerer-options.ts
Outdated
| registrar?: URI; | ||
|
|
||
| /** | ||
| * Determines when a re-REGISTER request is sent. The value should be specified as a percentage of the expiration time (between 1 and 50). |
src/api/registerer.ts
Outdated
| */ | ||
| export class Registerer { | ||
| private static readonly defaultExpires = 600; | ||
| private static readonly defaultRefreshFrequency = 50; |
There was a problem hiding this comment.
Maybe default to 99? For a 1 hour expiration this would be about 36 seconds before expiration.
src/api/registerer.ts
Outdated
|
|
||
| // Interval at which re-REGISTER requests are sent | ||
| this.refreshFrequency = this.options.refreshFrequency || Registerer.defaultRefreshFrequency; | ||
| if (this.refreshFrequency < 1 || this.refreshFrequency > 50) { |
src/api/registerer.ts
Outdated
| this.refreshFrequency = this.options.refreshFrequency || Registerer.defaultRefreshFrequency; | ||
| if (this.refreshFrequency < 1 || this.refreshFrequency > 50) { | ||
| throw new Error( | ||
| "Invalid refresh frequency. The value represents a percentage of the expiration time and should be between 1 and 50." |
src/api/registerer-options.ts
Outdated
|
|
||
| /** | ||
| * Determines when a re-REGISTER request is sent. The value should be specified as a percentage of the expiration time (between 1 and 50). | ||
| * @defaultValue 50 |
3e2d865 to
0c275ad
Compare
|
Now I see I had completely misunderstood your comment 😅 It should be fixed now. As for the default value, 99 is the closest to the current behavior... or otherwise values like 99.9 should also be allowed. |
|
This looks great now, thanks! And thank you @seanbright for the reviewing assist! |
as suggested in #870