Don't pop the command queue when PubSub messages are received #360

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

tleach commented Dec 31, 2012

I debugged this issue just before discovering @cxreg had independently logged this issue in #342 and submitted pull request #356 to fix it.

@cxreg's fix looks good, however I decided to submit this separate pull request for two reasons:

  1. @cxreg's fix seems to break a couple of tests (the extra checks to ensure the reply array contains at least one non-null item seem to be necessary to make them pass).
  2. I wrote a couple of extra unit tests to reproduce the issue and prove the fix addresses it. This adds a little more value.

Ready to merge checklist

  • test(s)
  • does the test fail without for PR but succeed once applied?
  • docs, if applicable
  • merges cleanly
  • coding style (4-space indents, looks similar to other code)

cxreg commented Jan 1, 2013

Your PR is more comprehensive than mine, so I will cancel mine. Thanks for adding the tests.

I would suggest squashing your fixup commits to make the history a little cleaner, but otherwise it looks good.

Contributor

tleach commented Jan 14, 2013

OK @cxreg, fixups have been squashed.

Owner

brycebaril commented Feb 17, 2013

👍

Contributor

DTrejo commented Feb 17, 2013

Hey thanks for the PR :)

Super close to merging this, but your tests don't fail! Even when your code is not in the branch.

Would you mind resubmitting tests* that fail without your commit, and succeed with it?
* You can gist it to me if that's easiest.

Thanks,
D

Contributor

tleach commented Feb 19, 2013

@DTrejo The tests do "fail" (at least when I run them) in the sense they hang due to reproduction of the bug (causing the next to never get called).

But you have a point, this should really come back as a proper fail result and not block other tests. If the tests were built on something like Mocha (btw any appetite for porting the tests to Mocha?) we'd get callback timeout failures out-of-the-box.

In the absence of that, I'll roll something myself and update the PR.

Tom Leach Detect is an incoming "reply" is in fact a pubsub message. If so, do …
…not pop the command queue.

This fixes an issue where the command queue gets popped prematurely by pubsub messages, leading to callbacks for those commands not being invoked.
93332e4
Contributor

tleach commented Feb 19, 2013

@DTrejo OK those tests should now fail. Let me know if you hit any more problems.

DTrejo closed this in 837cec3 Feb 24, 2013

Contributor

DTrejo commented Feb 24, 2013

Thank you very much tom!
D

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