Closed Bug 974410 Opened 6 years ago Closed 6 years ago

Add helper classes for I/O watchers

Categories

(Firefox OS Graveyard :: General, defect)

Other
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S2 (28feb)

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(4 files, 1 obsolete file)

We have several classes that listen for I/O on the I/O thread. They all shared the same patterns for fd handling and I/O. There should be some helper classes that implement these patterns in a generic. way
Summary: Add helper classes fir I/O watchers → Add helper classes for I/O watchers
Kyle, I noticed that we need to remove a file-descriptor watcher explicitly, before we can add it again to the I/O loop. Otherwise the I/O loop might get stuck in an endless loop. Did you ever encounter this problem?
Attachment #8380674 - Flags: review?(kyle)
Comment on attachment 8380674 [details] [diff] [review]
[01] Bug 974410: Added watcher classes for Unix file descriptors

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

::: ipc/moz.build
@@ +23,5 @@
>  if CONFIG['MOZ_B2G_RIL'] or CONFIG['MOZ_B2G_BT'] or CONFIG['MOZ_NFC'] or CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
>      DIRS += ['unixsocket']
>  
>  if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
> +    DIRS += ['keystore', 'netd', 'unixfd']

Not-even-a-nit: I'm not sure if it's worth planning ahead for mulet or not, but we may want this compilable onto desktop. Is there any reason it's just on gonk, outside of the fact that right now we only use it for services there?
Attachment #8380674 - Flags: review?(kyle) → review+
Comment on attachment 8380676 [details] [diff] [review]
[02] Bug 974410: Inherit UnixSocketImpl from UnixFdWatcher

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

::: ipc/unixsocket/UnixSocket.cpp
@@ +43,3 @@
>  {
>  public:
> +  UnixSocketImpl(MessageLoop* mIOLoop,

Is there a point where this won't be using the IO thread?
Attachment #8380676 - Flags: review?(kyle) → review+
Attachment #8380677 - Flags: review?(kyle) → review+
Comment on attachment 8380678 [details] [diff] [review]
[04] Bug 974410: Cleanup UnixSocketImpl

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

Wow, I was just about to ask for at least a rename of SetUpIO too, but this goes one better. :D
Attachment #8380678 - Flags: review?(kyle) → review+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #1)

> Kyle, I noticed that we need to remove a file-descriptor watcher explicitly,
> before we can add it again to the I/O loop. Otherwise the I/O loop might get
> stuck in an endless loop. Did you ever encounter this problem?

Oh. Hmmm. No, I never ran into it, but I think that's by sheer luck.
BTW, we'll have to figure out when this is landable since it affects a ton of important FxOS stuff but we're in 1.4 freeze now and it's not actually a bug fixer. I guess no one really touches UnixSocket so it probably won't hit conflicts.
Hi Kyle,

Thanks for the review.

> >  if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
> > +    DIRS += ['keystore', 'netd', 'unixfd']
> 
> Not-even-a-nit: I'm not sure if it's worth planning ahead for mulet or not,
> but we may want this compilable onto desktop. Is there any reason it's just
> on gonk, outside of the fact that right now we only use it for services
> there?

Thinking about it, not building the code on Desktop should prevent UnixSocket (and Desktop-BT) from building.
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #6)
> Comment on attachment 8380676 [details] [diff] [review]
> [02] Bug 974410: Inherit UnixSocketImpl from UnixFdWatcher
> 
> Review of attachment 8380676 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/unixsocket/UnixSocket.cpp
> @@ +43,3 @@
> >  {
> >  public:
> > +  UnixSocketImpl(MessageLoop* mIOLoop,
> 
> Is there a point where this won't be using the IO thread?

I don't think so. I just found it ugly to hard-code a call to XRE_GetMessageLoopForIO in the constructor. :D

The original reason why the unixfd classes keep a reference to their I/O loop is because of NetlinkPoller, which does the same thing. And it allows for stricter assertions, of course.
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #8)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #1)
> 
> > Kyle, I noticed that we need to remove a file-descriptor watcher explicitly,
> > before we can add it again to the I/O loop. Otherwise the I/O loop might get
> > stuck in an endless loop. Did you ever encounter this problem?
> 
> Oh. Hmmm. No, I never ran into it, but I think that's by sheer luck.

According to the comments in chromium's header files, this should work, but it doesn't in practice.

The bug happens if I call AddWatchers(WRITE_WATCHER) for a file descriptor that is already writable. IIRC there is an internal list of fds that are ready and adding the same watcher twice introduces a cyclic reference into this list.
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #9)
> BTW, we'll have to figure out when this is landable since it affects a ton
> of important FxOS stuff but we're in 1.4 freeze now and it's not actually a
> bug fixer. I guess no one really touches UnixSocket so it probably won't hit
> conflicts.

Good point. I will talk to sheriffs if and when this is land-able.
Changes to v1:

  - always build unixfd when unixsocket gets built
Attachment #8380674 - Attachment is obsolete: true
Attachment #8382041 - Flags: review+
Tomcat told me on irc that landing in b2g-inbound is not a problem, and the try run looks good as well. I'm going to land this.
Depends on: 979668
You need to log in before you can comment on or make changes to this bug.