Last Comment Bug 744453 - B2G RIL: Network Friendly / APN connection retry policy
: B2G RIL: Network Friendly / APN connection retry policy
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla15
Assigned To: Fernando R. Sela (no CC, needinfo please) [:frsela]
:
Mentors:
Depends on: 717122 744700
Blocks: b2g-3g
  Show dependency treegraph
 
Reported: 2012-04-11 09:55 PDT by Fernando R. Sela (no CC, needinfo please) [:frsela]
Modified: 2012-06-02 12:11 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to retry failed PDP context activation (4.82 KB, patch)
2012-04-16 05:55 PDT, Fernando R. Sela (no CC, needinfo please) [:frsela]
no flags Details | Diff | Review
Patch to retry failed PDP context activation (v2) (2.95 KB, patch)
2012-04-23 22:28 PDT, Fernando R. Sela (no CC, needinfo please) [:frsela]
no flags Details | Diff | Review
Patch to retry failed PDP context activation (v3) (5.95 KB, patch)
2012-05-08 18:20 PDT, Fernando R. Sela (no CC, needinfo please) [:frsela]
philipp: feedback+
Details | Diff | Review
Patch to retry failed PDP context activation (v4) (6.12 KB, patch)
2012-05-21 04:41 PDT, Fernando R. Sela (no CC, needinfo please) [:frsela]
philipp: review+
Details | Diff | Review
Patch to retry failed PDP context activation (v5) (nits corrected) (6.24 KB, patch)
2012-05-25 07:17 PDT, Fernando R. Sela (no CC, needinfo please) [:frsela]
philipp: review+
Details | Diff | Review
Patch to retry failed PDP context activation (v6) (5.39 KB, patch)
2012-05-31 03:13 PDT, Fernando R. Sela (no CC, needinfo please) [:frsela]
philipp: review-
Details | Diff | Review
Patch to retry failed PDP context activation (v7) (6.06 KB, patch)
2012-05-31 23:23 PDT, Fernando R. Sela (no CC, needinfo please) [:frsela]
philipp: review+
Details | Diff | Review

Description Fernando R. Sela (no CC, needinfo please) [:frsela] 2012-04-11 09:55:50 PDT
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
Comment 1 Fernando R. Sela (no CC, needinfo please) [:frsela] 2012-04-11 10:00:30 PDT
First implementation on: https://github.com/frsela/mozilla-central/tree/b2g-network_apnretry

I'll rebase with the Bug: 717122 last patches.
Comment 2 Fernando R. Sela (no CC, needinfo please) [:frsela] 2012-04-16 05:55:03 PDT
Created attachment 615312 [details] [diff] [review]
Patch to retry failed PDP context activation

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.
Comment 3 Fernando Jiménez Moreno [:ferjm] 2012-04-21 08:45:42 PDT
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 4 Fernando Jiménez Moreno [:ferjm] 2012-04-21 08:51:03 PDT
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 :))
Comment 5 Fernando R. Sela (no CC, needinfo please) [:frsela] 2012-04-23 22:28:28 PDT
Created attachment 617794 [details] [diff] [review]
Patch to retry failed PDP context activation (v2)

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
Comment 6 Philipp von Weitershausen [:philikon] 2012-04-25 19:41:24 PDT
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.
Comment 7 Fernando R. Sela (no CC, needinfo please) [:frsela] 2012-05-08 18:20:53 PDT
Created attachment 622242 [details] [diff] [review]
Patch to retry failed PDP context activation (v3)
Comment 8 Fernando Jiménez Moreno [:ferjm] 2012-05-14 01:57:30 PDT
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 9 Philipp von Weitershausen [:philikon] 2012-05-17 17:47:01 PDT
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.
Comment 10 Fernando R. Sela (no CC, needinfo please) [:frsela] 2012-05-21 04:41:25 PDT
Created attachment 625607 [details] [diff] [review]
Patch to retry failed PDP context activation (v4)
Comment 11 Philipp von Weitershausen [:philikon] 2012-05-23 19:08:11 PDT
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.
Comment 12 Fernando R. Sela (no CC, needinfo please) [:frsela] 2012-05-25 07:17:35 PDT
Created attachment 627221 [details] [diff] [review]
Patch to retry failed PDP context activation (v5) (nits corrected)
Comment 13 Philipp von Weitershausen [:philikon] 2012-05-29 15:24:12 PDT
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)
Comment 14 Philipp von Weitershausen [:philikon] 2012-05-30 18:35:17 PDT
Fernando, can you please rebase your batch onto the latest mozilla-central so we can land it. Thanks!
Comment 15 Fernando R. Sela (no CC, needinfo please) [:frsela] 2012-05-31 03:13:53 PDT
Created attachment 628656 [details] [diff] [review]
Patch to retry failed PDP context activation (v6)

Thank you Rebased.
Comment 16 Fernando R. Sela (no CC, needinfo please) [:frsela] 2012-05-31 03:19:03 PDT
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 17 Philipp von Weitershausen [:philikon] 2012-05-31 14:47:30 PDT
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.
Comment 18 Philipp von Weitershausen [:philikon] 2012-05-31 14:48:24 PDT
(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.
Comment 19 Fernando R. Sela (no CC, needinfo please) [:frsela] 2012-05-31 23:23:13 PDT
Created attachment 629081 [details] [diff] [review]
Patch to retry failed PDP context activation (v7)

As discussed today in IRC, this version adds the new shutdown() method on RILNetworkInterface which is called from xpcom-shutdown.
Comment 20 Philipp von Weitershausen [:philikon] 2012-06-01 14:13:53 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/824f8d575f0f
Comment 21 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-02 12:11:11 PDT
https://hg.mozilla.org/mozilla-central/rev/824f8d575f0f

Note You need to log in before you can comment on or make changes to this bug.