Configurable socket threads

RESOLVED FIXED in Firefox 41

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

unspecified
2.2 S14 (12june)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(3 attachments)

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).
Created attachment 8613437 [details] [diff] [review]
[01] Bug 1168806: Configurable I/O thread for socket IPC classes
Attachment #8613437 - Flags: review?(kyle)
Created attachment 8613438 [details] [diff] [review]
[02] Bug 1168806: Configurable consumer thread for socket IPC classes
Attachment #8613438 - Flags: review?(kyle)
Created attachment 8613439 [details] [diff] [review]
[03] Bug 1168806: Use 'consumer thread' in socket interface
Attachment #8613439 - Flags: review?(kyle)
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?
Attachment #8613437 - Flags: review?(kyle) → review+
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?
Attachment #8613438 - Flags: review?(kyle) → review+
> 
> 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.
> ::: 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.
Blocks: 1170466
https://hg.mozilla.org/mozilla-central/rev/ef212da75478
https://hg.mozilla.org/mozilla-central/rev/075e96a57701
https://hg.mozilla.org/mozilla-central/rev/fd7c97d41cf3
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S14 (12june)
You need to log in before you can comment on or make changes to this bug.