1165776 - Support "run in new thread" for dhcpRequest in NetworkWorker

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Henry Chang [:hchang]]

Current nsINetworkService functions will be executed in one worker thread.
For some long waiting time commands like dhcpRequest, we should add the 
capability like 'run in new thread' to those ones.

Comment 1

[Henry Chang [:hchang]]

Attachment: Bug1165776.patch

Comment 2

[Henry Chang [:hchang]]

Comment on attachment 8606825 [details] [diff] [review]
Bug1165776.patch

Hi Fabrice, 

Could you please help review this patch? It spins a new thread to
run the function "dhcp_request" which may take a long time. For now
there is only one command which would tag with 'runInNewThread' so
that I spawn a new thread for each request. When things become more
complicated in the future, I'd like to use a thread pool to reduce
thread creation overhead. But for now, I just make things as simple
as possible. 

Thanks!

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 8606825 [details] [diff] [review]
Bug1165776.patch

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

Looks fine for now, but please file the followup to use a thread pool.

::: dom/webidl/NetworkOptions.webidl
@@ +55,5 @@
>    long gateway_long;                  // for "ifc_configure".
>    long dns1_long;                     // for "ifc_configure".
>    long dns2_long;                     // for "ifc_configure".
> +
> +  boolean runInNewThread = false;

please add a comment.

Comment 4

[Henry Chang [:hchang]]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> Comment on attachment 8606825 [details] [diff] [review]
> Bug1165776.patch
> 
> Review of attachment 8606825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine for now, but please file the followup to use a thread pool.
> 
> ::: dom/webidl/NetworkOptions.webidl
> @@ +55,5 @@
> >    long gateway_long;                  // for "ifc_configure".
> >    long dns1_long;                     // for "ifc_configure".
> >    long dns2_long;                     // for "ifc_configure".
> > +
> > +  boolean runInNewThread = false;
> 
> please add a comment.

Thanks Fabrice! I'll file a followup bug to track.

Comment 5

[Henry Chang [:hchang]]

Attachment: Bug1165776 v2 (carry r+ from Fabrice's review)

Comment 6

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7830114e635e

Comment 7

[Ryan VanderMeulen [:RyanVM]]

webidl changes need DOM peer review

Comment 8

[Henry Chang [:hchang]]

Comment on attachment 8607295 [details] [diff] [review]
Bug1165776 v2 (carry r+ from Fabrice's review)

Hi Blake,

Could you please have a webidl change review? It's only for internal use
and we add a flag to control if the command should be run in a new thread
or in the common worker thread.

Thanks!

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8607295 [details] [diff] [review]
Bug1165776 v2 (carry r+ from Fabrice's review)

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

::: dom/system/gonk/NetworkWorker.cpp
@@ +213,5 @@
> +  if (NetworkParams.mRunInNewThread) {
> +    // TODO: Bug 1166131 - Use a thread pool to process commands
> +    //       tagged with 'runInNewThread'.
> +    nsCOMPtr<nsIThread> thread;
> +    NS_NewThread(getter_AddRefs(thread), runnable);

This leaks the new thread. All threads must have nsIThread::Shutdown called on them. If we limit the new thread to DHCP only, we could simply create the new thread in NetworkWorker::Start and shut it down in NetworkWorker::Shutdown and not have to worry about thread pools.

Also, please use NS_NewNamedThread so we can more easily identify it in debuggers and memory reporters.

::: dom/webidl/NetworkOptions.webidl
@@ +60,5 @@
> +  // thread. However, for some native commands like 'dhcp_request' which
> +  // may block the worker thread for a long time. The caller can set
> +  // this flag to specifiy the internal to run this command in a separate
> +  // thread.
> +  boolean runInNewThread = false;

How general is this mechanism going to be? DHCP should definitely be on its own thread, but it seems like it'd be easier to simply switch on the command name and use a different thread for dhcpRequest instead of having a generic switch to run on a new thread.

If you really don't like that, I'd be OK keeping this. I want to make sure, though.

Comment 10

[Henry Chang [:hchang]]

Hi Blake,

Sorry for late response and thanks for your review.

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #9)
> Comment on attachment 8607295 [details] [diff] [review]
> Bug1165776 v2 (carry r+ from Fabrice's review)
> 
> Review of attachment 8607295 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkWorker.cpp
> @@ +213,5 @@
> > +  if (NetworkParams.mRunInNewThread) {
> > +    // TODO: Bug 1166131 - Use a thread pool to process commands
> > +    //       tagged with 'runInNewThread'.
> > +    nsCOMPtr<nsIThread> thread;
> > +    NS_NewThread(getter_AddRefs(thread), runnable);
> 
> This leaks the new thread. All threads must have nsIThread::Shutdown called
> on them. If we limit the new thread to DHCP only, we could simply create the
> new thread in NetworkWorker::Start and shut it down in
> NetworkWorker::Shutdown and not have to worry about thread pools.
> 

I was also worried about the leak until finding out a couple of places doing the 
same things like [1]. But after digging a little bit more, |Shutdown| is 
required to call to release the resource as you said.

[1] http://hg.mozilla.org/mozilla-central/file/b0a507af2b4a/toolkit/identity/IdentityCryptoService.cpp#l332

> Also, please use NS_NewNamedThread so we can more easily identify it in
> debuggers and memory reporters.
> 
> ::: dom/webidl/NetworkOptions.webidl
> @@ +60,5 @@
> > +  // thread. However, for some native commands like 'dhcp_request' which
> > +  // may block the worker thread for a long time. The caller can set
> > +  // this flag to specifiy the internal to run this command in a separate
> > +  // thread.
> > +  boolean runInNewThread = false;
> 
> How general is this mechanism going to be? DHCP should definitely be on its
> own thread, but it seems like it'd be easier to simply switch on the command
> name and use a different thread for dhcpRequest instead of having a generic
> switch to run on a new thread.
> 
> If you really don't like that, I'd be OK keeping this. I want to make sure,
> though.

I have no strong reason to keep this. So I am going to use the task name
to determine which thread to dispatch. There will be a function such as  
|NetworkUtils::ShouldRunOutOfWorkerThread| to be called by NetworkWorker.

Thanks!

Comment 11

[Henry Chang [:hchang]]

No longer working on this.

Comment 12

[BMO Automation]

Firefox OS is not being worked on