poll-daemon

Merge lp://staging/~nikwen/account-polld/fix-timeout-handler into lp://staging/~ubuntu-push-hackers/account-polld/trunk

  1. fix-timeout-handler
  2. Merge into trunk
Proposed by Niklas Wenzel
Status: Merged
Approved by: Jonas G. Drange
Approved revision: 139
Merged at revision: 145
Proposed branch: lp://staging/~nikwen/account-polld/fix-timeout-handler
Merge into: lp://staging/~ubuntu-push-hackers/account-polld/trunk
Diff against target: 119 lines (+25/-34)
4 files modified
qtcontact/contacts.go (+14/-25)
qtcontact/qtcontacts.cpp (+9/-7)
qtcontact/qtcontacts.h (+1/-1)
qtcontact/qtcontacts.hpp (+1/-1)
To merge this branch: bzr merge lp://staging/~nikwen/account-polld/fix-timeout-handler
Reviewer Review Type Date Requested Status
Jonas G. Drange (community) Approve
Review via email: mp+273351@code.staging.launchpad.net

Commit message

Fix the timeout logic in the qtcontacts module (part of LP: #1498214)

Description of the change

Fix the timeout logic in the qtcontacts module (part of LP: #1498214)

(I split the fix for the linked bug report into two MPs as it makes the code much easier to understand. It consists of two individual issues anyway.)

To post a comment you must log in.
Revision history for this message
Niklas Wenzel (nikwen) wrote :

I found an issue in branch 2 so there is just this MP for now until I fix the other one.

Revision history for this message
Niklas Wenzel (nikwen) wrote :

To clarify: Both branches work perfectly now. :)

Revision history for this message
Jonas G. Drange (jonas-drange) wrote :

Looks good, one small nitpick :)

review: Needs Information
Revision history for this message
Niklas Wenzel (nikwen) wrote :

Thank you for reviewing this. :)

The line you commented on really does nothing besides removing an unnecessary go routine. No issue addressed by it. ;)

Please note though that this MP only fixes the timeout handling code. The real issue is fixed by https://code.launchpad.net/~nikwen/account-polld/fix-qtcontacts-timeout-error/+merge/273356. ;)

Revision history for this message
Jonas G. Drange (jonas-drange) wrote :

Apologies Niklas, but we'll have to remove that diff on line 8-11 unless someone else can approve of it. I can't simply for the reason that 1) I don't know enough about the code that's being changed and 2) there are no tests specifically testing this path of code so I have a hard time reasoning about potential fallout/deadlocks.

review: Needs Fixing
lp://staging/~nikwen/account-polld/fix-timeout-handler updated
139. By Niklas Wenzel

Revert removing a go routine

Revision history for this message
Niklas Wenzel (nikwen) wrote :

No problem. Done. ;)

Revision history for this message
Jonas G. Drange (jonas-drange) wrote :

I can't repro, but the code looks good and works well. Maybe it's a race I'm not able to reproduce. Let's land it. :)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches