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

Keep old clients from removing message rate delay #252

Closed

Conversation

digitalcircuit
Copy link
Contributor

@digitalcircuit digitalcircuit commented Sep 22, 2016

In short

  • Refuse zero message delays in setMessageRateDelay
    • Stops old clients from setting message rate delay to 0.00 s when creating networks
    • Mimics error checking of setMessageRateBurstSize
    • Enforces core-side; Quassel UI already blocks zero values
    • Removing rate limits by setting Unlimited to true still works
Criteria Rank Reason
Impact ★★☆ 2/3 User-facing, avoids corrupting custom rate-limit configuration
Risk ★☆☆ 1/3 More log messages with old clients, not accepting valid changes
Intrusiveness ★☆☆ 1/3 Small set of changes, shouldn't interfere with other pull requests

Testing

Steps

  1. Start a core running 0.13-git-master
  2. Connect with Quassel client 0.12.4
  3. Create a new network, save it
  4. Connect with Quassel client 0.13-git-master
  5. In newer client, check the Network → Connection tab

Results

  • Without fix
    • Message delay is set to 0s (shown as 0.01s due to UI limits)
  • With fix
    • Message delay is set to 2.2s, the Quassel default

In both cases, custom rate limits aren't used, however this becomes an issue when one first tries to use custom rate-limits. They might assume Quassel has almost no delay between messages by default.

Example

Before - custom rate limit wrongly not set to defaults

Settings -> Network -> Connection: 0.01s wrong non-default message rate delay

After - custom rate limit properly set to defaults

Settings -> Network -> Connection: 2.20s proper default message rate delay

After - core log

Old client creates a new network on a new core

2016-09-21 19:46:07 Debug: Received invalid setMessageRateBurstSize data - message burst size must be non-zero positive, given 0 
2016-09-21 19:46:07 Debug: Received invalid setMessageRateDelay data - message delay must be non-zero positive, given 0

Add a guard against zero message delay in setMessageRateDelay,
following the error-checking of setMessageRateBurstSize.  This blocks
old clients from setting message rate delay to 0 seconds for newly-
created networks, improperly overriding the default of 2.2 seconds.

This affects creating a network in an old client, switching to a new
client, then enabling custom rate-limits.

The supported method for removing rate limits by setting Unlimited to
true still works.
@Sput42 Sput42 closed this in 4d0f519 Oct 13, 2016
@digitalcircuit digitalcircuit deleted the fix-custom-rate-defaults branch October 13, 2016 20:25
javierllorente pushed a commit to javierllorente/quassel that referenced this pull request Jan 6, 2017
Add a guard against zero message delay in setMessageRateDelay,
following the error-checking of setMessageRateBurstSize.  This blocks
old clients from setting message rate delay to 0 seconds for newly-
created networks, improperly overriding the default of 2.2 seconds.

This affects creating a network in an old client, switching to a new
client, then enabling custom rate-limits.

The supported method for removing rate limits by setting Unlimited to
true still works.

Resolves quasselGH-252.
javierllorente pushed a commit to javierllorente/quassel that referenced this pull request Jan 6, 2017
Add a guard against zero message delay in setMessageRateDelay,
following the error-checking of setMessageRateBurstSize.  This blocks
old clients from setting message rate delay to 0 seconds for newly-
created networks, improperly overriding the default of 2.2 seconds.

This affects creating a network in an old client, switching to a new
client, then enabling custom rate-limits.

The supported method for removing rate limits by setting Unlimited to
true still works.

Resolves quasselGH-252.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant