Closed Bug 1168806 Opened 5 years ago Closed 5 years ago

Configurable socket threads

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
2.2 S14 (12june)
Tracking Status
firefox41 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(3 files)

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 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+
Attachment #8613439 - 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.
You need to log in before you can comment on or make changes to this bug.