1168806 - Configurable socket threads

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

The socket I/O classes currently transfer data between main thread and I/O thread.

The threads should be configurable. This might allow for more flexible thread setups, like doing I/O on a worker thread (e.g., for RIL).

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=509c5acd04f4

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1168806: Configurable I/O thread for socket IPC classes

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1168806: Configurable consumer thread for socket IPC classes

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1168806: Use 'consumer thread' in socket interface

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8613437 [details] [diff] [review]
[01] Bug 1168806: Configurable I/O thread for socket IPC classes

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

::: ipc/unixsocket/StreamSocket.cpp
@@ +523,5 @@
>  }
>  
>  nsresult
> +StreamSocket::Connect(UnixSocketConnector* aConnector, int aDelayMs,
> +                      MessageLoop* aIOLoop)

Nit that happens a lot of places around this patch: Sometimes we assert on raw pointers, sometimes we don't. Do we want to assert on aConnector/aIOLoop/other raw pointers in all of the places these show up?

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8613438 [details] [diff] [review]
[02] Bug 1168806: Configurable consumer thread for socket IPC classes

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

::: dom/bluetooth/bluedroid/BluetoothSocket.cpp
@@ +808,5 @@
>  }
>  
>  void
>  BluetoothSocket::OnConnectSuccess()
>  {

Now that we have consumer threads, will we ever want to start/stop threads on the main thread? If not, should we check for that?

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> 
> Nit that happens a lot of places around this patch: Sometimes we assert on
> raw pointers, sometimes we don't. Do we want to assert on
> aConnector/aIOLoop/other raw pointers in all of the places these show up?

Too few exceptions and you run into bugs, too many and the code becomes unreadable. :/

In general, my policy is to put asserts into the methods of a public interface, such as |StreamSocket|, and constructors, but not internal methods. So once something is set up, I expect it to work. There might be exceptions to the rule, though.

I'm open to any suggestions.

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> ::: dom/bluetooth/bluedroid/BluetoothSocket.cpp
> @@ +808,5 @@
> >  }
> >  
> >  void
> >  BluetoothSocket::OnConnectSuccess()
> >  {
> 
> Now that we have consumer threads, will we ever want to start/stop threads
> on the main thread? If not, should we check for that?

The patches just generalize the code we already have. I don't think that the socket classes should do thread management. All threads are expected to be around while an instance is in use.

The 'consumer thread' is actually a producer and consumer. I couldn't come up with a better name than 'consumer'. It's what the main thread originally has been used for. That means that a consumer thread also has to open/close, construct/destruct its instances of a socket class.

For an error check: If there's a way to get a message when a thread gets deleted, we could let the socket classes register themselves for this message and then fail when it happens.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/ef212da75478
https://hg.mozilla.org/integration/b2g-inbound/rev/075e96a57701
https://hg.mozilla.org/integration/b2g-inbound/rev/fd7c97d41cf3

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks!

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=fd7c97d41cf3

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ef212da75478
https://hg.mozilla.org/mozilla-central/rev/075e96a57701
https://hg.mozilla.org/mozilla-central/rev/fd7c97d41cf3