Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cupsEnumDests to finish when receiving the ALL_FOR_NOW avahi event #4989

Closed

Conversation

antlarr
Copy link

@antlarr antlarr commented Apr 11, 2017

cupsEnumDests listens for avahi answers for msec milliseconds (where
msec is given as a parameter to cupsEnumDests). avahi sends an
ALL_FOR_NOW event [1] when "more records will probably not show up in
the near future". This means that from the reception of ALL_FOR_NOW
to the timeout triggering, cupsEnumDests basically does nothing but
block the caller. Also, since the caller can't know how long it will
take cupsEnumDests to receive all information from avahi, this means
developers are encouraged to use "big enough" values for msec,
thus blocking their calling threads unnecessarily.

This patch makes cupsEnumDests react to the ALL_FOR_NOW event and
finish as soon as all avahi caches have been read and all static
servers have been queried.

No behavior is changed when HAVE_DNSSD is defined.

[1] http://www.avahi.org/doxygen/html/defs_8h.html#af7ff3b95259b3441a282b87d82eebd87a430e1134047305e7b892e784cf72f815

cupsEnumDests listens for avahi answers for `msec` milliseconds (where
`msec` is given as a parameter to cupsEnumDests). avahi sends an
ALL_FOR_NOW event [1] when "more records will probably not show up in
the near future". This means that from the reception of ALL_FOR_NOW
to the timeout triggering, cupsEnumDests basically does nothing but
block the caller. Also, since the caller can't know how long it will
take cupsEnumDests to receive all information from avahi, this means
developers are encouraged to use "big enough" values for `msec`,
thus blocking their calling threads unnecessarily.

This patch makes cupsEnumDests react to the ALL_FOR_NOW event and
finish as soon as all avahi caches have been read and all static
servers have been queried.

No behavior is changed when HAVE_DNSSD is defined.

[1] http://www.avahi.org/doxygen/html/defs_8h.html#af7ff3b95259b3441a282b87d82eebd87a430e1134047305e7b892e784cf72f815
@michaelrsweet
Copy link
Collaborator

mDNSResponder has a similar mechanism, however in practice it has not proven reliable since all it means is that there are no more responses queued up... Will think about how we can make this better...

@michaelrsweet michaelrsweet self-assigned this Apr 12, 2017
@michaelrsweet michaelrsweet added this to the 2.2 milestone Apr 12, 2017
@michaelrsweet
Copy link
Collaborator

OK, I have a better (generic) fix for this that uses some of the tricks from the dnssd backend to return early when we know we have all responses. Doing some more testing and will push a change once I'm satisfied.

@antlarr
Copy link
Author

antlarr commented Apr 19, 2017

Cool, thanks for working on this. Do you have your solution published anywhere so I can test it too?

@michaelrsweet
Copy link
Collaborator

Sorry, no, I'm just working directly on master for this change. But it shouldn't be too much longer before I push it to Github...

michaelrsweet pushed a commit that referenced this pull request Apr 19, 2017
…ave been

discovered (Issue #4989)

Also update the code to generate the same queue names as cupsd does for IPP
Everywhere printers.
@michaelrsweet
Copy link
Collaborator

[master a2187a6] Update cupsEnumDests implementation to return early if all printers have been discovered (Issue #4989)

@michaelrsweet
Copy link
Collaborator

Please verify the fix in Github resolves the problems you were having.

@antlarr
Copy link
Author

antlarr commented Apr 20, 2017

I added a couple of comments to your commit. Do you want me to modify my PR to fix those? Or do you prefer to work on them yourself?

@michaelrsweet
Copy link
Collaborator

@antlarr I'm doing follow-up testing on a few Linux distros now (first pass mainly tested macOS) and had found the issues you have reported in the comments to the commit. I have some more changes for Avahi coming...

michaelrsweet added a commit that referenced this pull request Apr 20, 2017
Also fix timeouts to track elapsed time so the timeout is more accurate.
@michaelrsweet
Copy link
Collaborator

OK, just pushed more changes that seem to make Ubuntu happy.

@antlarr
Copy link
Author

antlarr commented Apr 20, 2017

Perfect, with a2187a6, 657c5b5 and 3fae3b3 now it works fine.

Thanks, I'll include those three commits in openSUSE Leap/Tumbleweed and SUSE Linux Enterprise. I'll close this pull request if you think that's ok now.

@michaelrsweet
Copy link
Collaborator

Yes, I'll probably keep testing on our other supported platforms (Windows in particular) but you should be good to go on Linux.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants