Closed Bug 744453 Opened 12 years ago Closed 12 years ago

B2G RIL: Network Friendly / APN connection retry policy

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: frsela, Assigned: frsela)

References

Details

Attachments

(1 file, 6 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux i686; rv:11.0) Gecko/20100101 Firefox/11.0
Build ID: 20120312181643

Steps to reproduce:

We need to create a Network Friendly Manager;

We are looking for: Minimizing network impact, Improving resources consumption,
Obtaining better device performance.

First patch: If the APN connection fails (low coverage, net problems, APN URL error, ...) the RIL shall retry the connection with increasing times
Depends on: 717122
OS: Linux → Gonk
Hardware: x86 → ARM
First implementation on: https://github.com/frsela/mozilla-central/tree/b2g-network_apnretry

I'll rebase with the Bug: 717122 last patches.
Assignee: nobody → frsela
This patch enables reconnection process when the PDP context creation failed (due to a APN URL error, no coverage, ...)

This one, has a "simple" retry policy. First time: 10 secs, Next 30, next 120, ...

It's required to patch over the 717122 last patches.

Also, I added a callback notification on ril_worker since in 727319 this had been removed. Note that this callback only works on Data Connection failures.
Attachment #615312 - Flags: review?(philipp)
Comment on attachment 615312 [details] [diff] [review]
Patch to retry failed PDP context activation

Review of attachment 615312 [details] [diff] [review]:
-----------------------------------------------------------------

With Philipp´s permission, I´d like to make some comments before his revision.

::: b2g/app/b2g.js
@@ +486,1 @@
>  

IMO this preferences shouldn´t be on the patch. This is only part of your tests and I guess it only work with TEF SIMs

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +45,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  
> +// Event timer for connection retries
> +let timer = Components.classes["@mozilla.org/timer;1"].createInstance(Components.interfaces.nsITimer);
> +

You can use `Cc` for `Components.classes`.

The line length must be 80 character or less. Also for the rest of the patch, please.

@@ +50,4 @@
>  var RIL = {};
>  Cu.import("resource://gre/modules/ril_consts.js", RIL);
>  
> +const DEBUG = true; // set to true to see debug messages

Remember to leave it to `false` on the final patch, please.

@@ +224,5 @@
>          this.radioState.type = RIL.GECKO_RADIO_TECH[state.radioTech] || null;
>          this.notifyRadioStateChanged();
>          break;
> +      case "dataregistrationerror":
> +        debug("Received error message: " + JSON.stringify(message));

Please, use `if (DEBUG)` for every `debug()` call. Also for the rest of the patch, please.

@@ +1210,4 @@
>    registeredAsDataCallCallback: false,
>    registeredAsNetworkInterface: false,
>    connecting: false,
> +  retrytimer: 0,

nit: retryTimer

@@ +1256,5 @@
> +      case 0: this.retrytimer = 10; break;
> +      case 10: this.retrytimer = 30; break;
> +      case 30: this.retrytimer = 120; break;
> +      default: this.retrytimer *= 2;
> +    }

https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Control_structures

@@ +1258,5 @@
> +      case 30: this.retrytimer = 120; break;
> +      default: this.retrytimer *= 2;
> +    }
> +    debug(" -> Retry Timer (secs): " + this.retrytimer);
> +    timer.initWithCallback(this,this.retrytimer*1000, Components.interfaces.nsITimer.TYPE_ONE_SHOT);

You can use `Ci` for `Components.interfaces`.

nit: spaces between comas and operators.

We problably want to notify the user about the reconnection attempts and expose a way to cancel it.

::: dom/system/gonk/ril_worker.js
@@ +482,1 @@
>      } else if (response_type == RESPONSE_TYPE_UNSOLICITED) {

IMO the `Buf` object is not the best place for this. The `RIL` object looks like a better place for me (RIL[REQUEST_SETUP_DATA_CALL]).
Comment on attachment 615312 [details] [diff] [review]
Patch to retry failed PDP context activation

Review of attachment 615312 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/ril_worker.js
@@ +476,5 @@
>        }
> +      // On Data Call Registration error, we shall notify caller
> +      if (request_type == REQUEST_SETUP_DATA_CALL && error) {
> +        options.type = "dataregistrationerror";
> +        RIL.sendDOMMessage(options);

On my last comment, I meant this piece of code.

(Sorry, it´s my first Splinter review :))
Modified version with Fernando's comments.
Also, a new retry policy implemented; based on the function:

    next_retry_time=A*number_of_retries^2+B
Attachment #615312 - Attachment is obsolete: true
Attachment #615312 - Flags: review?(philipp)
Attachment #617794 - Flags: review?
Attachment #617794 - Flags: review? → review?(philipp)
Comment on attachment 617794 [details] [diff] [review]
Patch to retry failed PDP context activation (v2)

Review of attachment 617794 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! I have a little bit of a hard time trying to understand the reasoning for the retry. If it failed the first time, why would it all of a sudden work seconds or minutes later? At the very least, I think we should look at the fail cause to get an idea *why* the data call failed and then act accordingly. Also might want to bubble some of it to content events so that the UI can display useful error messages (e.g. auth failure, etc.). I have the feeling that we probably want to have a discussion about what sort of behaviour we want and where that behaviour shoudl live. Perhaps some of it should live in content (e.g. Gaia)...?

In any case, some general issues with this patch:

* magic numbers that aren't explained (we prefer constants that have descriptive names at the very least, but ideally are also accompanied by explanatory comments.)
* confusing variable naming and comments ("retryFunc*" are in fact numbers, not functions, the "notify" method is unused and not documented, etc.)
* lots of coding style violation (Fernando already pointed them out, so I'm not going to repeat them)

::: dom/system/gonk/ril_worker.js
@@ +477,5 @@
> +      // On Data Call Registration error, we shall notify caller
> +      if (request_type == REQUEST_SETUP_DATA_CALL && error) {
> +        options.type = "dataregistrationerror";
> +        RIL.sendDOMMessage(options);
> +      }

Fernando already pointed this out, but you didn't address this: This is totally the wrong place. Please handle this case in RIL[REQUEST_SETUP_DATA_CALL].

Also, we should do the same thing that we do with voice calls (bug 712944) and request the last fail cause for the data call. We can then give the main thread an informed response (could even bubble this up to the UI in WebMobileConnection.) So this isn't so much a "data registration error" as it is a "data call error", so I suggest calling this message "dataCallError" or so.
Attachment #617794 - Flags: review?(philipp)
Attachment #617794 - Attachment is obsolete: true
Attachment #622242 - Flags: review?(philipp)
Comment on attachment 622242 [details] [diff] [review]
Patch to retry failed PDP context activation (v3)

Review of attachment 622242 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/ril_worker.js
@@ +2475,5 @@
>  
>  RIL[REQUEST_SETUP_DATA_CALL] = function REQUEST_SETUP_DATA_CALL(length, options) {
>    if (options.rilRequestError) {
> +    // On Data Call Registration error, we shall notify caller
> +    this.sendDOMMessage({type: "dataregistrationerror"});

I agree with Philipp that this probably looks more like a "datacallerror" :)
Comment on attachment 622242 [details] [diff] [review]
Patch to retry failed PDP context activation (v3)

Review of attachment 622242 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good. Almost there! :)

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +45,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  
> +// Event timer for connection retries
> +let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +

It would be cleaner to make this a property of the RILNetworkInterface object. In any case it needs to be nulled at shutdown (see observe()) to avoid leaks.

::: dom/system/gonk/ril_consts.js
@@ +1392,5 @@
> + * Retry funcion: time = A * numer_of_retries^2 + B
> + */
> +const NETWORK_APNRETRY_FACTOR = 8;
> +const NETWORK_APNRETRY_ORIGIN = 3;
> +const NETWORK_APNRETRY_MAXRETRIES = 10;

These are not used in ril_worker.js, so they should be in RadioInterfaceLayer.js instead of here.

Please also document the unit... is it seconds? milliseconds? I have no idea!

::: dom/system/gonk/ril_worker.js
@@ +2475,5 @@
>  
>  RIL[REQUEST_SETUP_DATA_CALL] = function REQUEST_SETUP_DATA_CALL(length, options) {
>    if (options.rilRequestError) {
> +    // On Data Call Registration error, we shall notify caller
> +    this.sendDOMMessage({type: "dataregistrationerror"});

Definitely "datacallerror", as ferjm pointed out.
Attachment #622242 - Flags: review?(philipp) → feedback+
Attachment #622242 - Attachment is obsolete: true
Attachment #625607 - Flags: review?(philipp)
Comment on attachment 625607 [details] [diff] [review]
Patch to retry failed PDP context activation (v4)

Review of attachment 625607 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed. Thanks!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1267,5 @@
> +  NETWORK_APNRETRY_MAXRETRIES: 10,
> +
> +  // Event timer for connection retries
> +  timer: Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer),
> +

Please lazily create it when you need it (seems like in `reset()`).

@@ +1364,5 @@
> +    // We will retry the connection in increasing times
> +    // based on the function: time = A * numer_of_retries^2 + B
> +    if (this.apnRetryCounter >= RILNetworkInterface.NETWORK_APNRETRY_MAXRETRIES) {
> +      this.apnRetryCounter = 0;
> +      debug("Too many retries - STOP");

Too many retries of what? Debug messages are only useful when they're clear :)

@@ +1370,5 @@
> +    }
> +
> +    apnRetryTimer = RILNetworkInterface.NETWORK_APNRETRY_FACTOR *
> +                    (this.apnRetryCounter * this.apnRetryCounter) +
> +                    RILNetworkInterface.NETWORK_APNRETRY_ORIGIN;

You can use `this` instead of `RILNetworkInterface` here.
Attachment #625607 - Flags: review?(philipp) → review+
Comment on attachment 627221 [details] [diff] [review]
Patch to retry failed PDP context activation (v5) (nits corrected)

Looks good, thanks!

Next time please format your commit message according to the Mozilla conventions (Bug NNN - Bug summary. r=reviewer)
Attachment #627221 - Flags: review?(philipp) → review+
Fernando, can you please rebase your batch onto the latest mozilla-central so we can land it. Thanks!
Thank you Rebased.
Attachment #627221 - Attachment is obsolete: true
Attachment #628656 - Flags: review?(philipp)
Thanks Philipp, I also added that when we finish trying reconnections, I also remove the timer instance (timer = null)

I'm thinking that it could be better to destroy the timer each retry and create a new one if needed? We'll have a little overhead creating it each time, but we assure that we remove it when it's not needed. WDYT?
Comment on attachment 628656 [details] [diff] [review]
Patch to retry failed PDP context activation (v6)

Review of attachment 628656 [details] [diff] [review]:
-----------------------------------------------------------------

You failed to carry over the the line of code that clears the timer on "xpcom-shutdown". Please start over and make sure you rebase the whole patch. I suggest diffing the two patches to make sure you didn't accidentally forget something.
Attachment #628656 - Flags: review?(philipp) → review-
(In reply to Fernando R. Sela from comment #16)
> I'm thinking that it could be better to destroy the timer each retry and
> create a new one if needed? We'll have a little overhead creating it each
> time, but we assure that we remove it when it's not needed. WDYT?

Keeping it around is fine, perhaps even preferred since it saves us pointless cycle collections.
As discussed today in IRC, this version adds the new shutdown() method on RILNetworkInterface which is called from xpcom-shutdown.
Attachment #628656 - Attachment is obsolete: true
Attachment #629081 - Flags: review?(philipp)
Attachment #629081 - Flags: review?(philipp) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/824f8d575f0f
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/824f8d575f0f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: