fix(close): ensure in-flight messages are drained #952

jeffijoe · 2020-04-09T18:00:21Z

Track in-flight requests and add an onDrain deferred that resolves when all in-flight ack/modAcks are done.

Fixes #951

googlebot · 2020-04-09T18:00:25Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

jeffijoe · 2020-04-09T18:01:45Z

@googlebot I signed it!

googlebot · 2020-04-09T18:01:47Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

feywind · 2020-04-10T21:24:06Z

Thanks for the contribution, @jeffijoe ! I'll review in just a sec.

feywind · 2020-04-10T21:42:02Z

test/message-queues.ts

+        const origSendBatch = patchedQueue._sendBatch;
+        const log: string[] = [];
+        const sendDone = defer();
+        patchedQueue._sendBatch = async function _sendBatch(


I wonder if this might be done using sandbox.stub just to be consistent with the rest of the tests. I'm going to ask someone who knows a bit more about Sinon than me.

Do you mean just the patching of the method?

Yep! Sorry for not being clear there. On second thought, I think it's not a big deal because you're making your own MessageQueue anyway.

I wanted to stay consistent with the rest of the source but I was not sure how else I would pull this off. Do you have any suggestions?

I think you could probably use the sandbox on the existing messageQueue._sendBatch, if you wanted to be a bit more consistent with the other tests. That might be nice if we change something about how the "global" MessageQueue is wired up under describe later, but I don't think it's worth spending much time on.

I changed it and rebased to tip of master but now other unrelated tests are failing. Is master healthy right now?

It should be, but we have been tracking down some flaky tests for a bit, too. I'll look.

jeffijoe · 2020-04-13T13:11:58Z

If you are wondering about the force-pushes, I am just rebasing to latest master.

feywind · 2020-04-13T20:17:53Z

Yep, no worries on the rebase!

feywind · 2020-04-13T21:25:39Z

I think the rebase is actually confusing our CI bots, so you might want to just use merge to get the tests running automatically.

jeffijoe · 2020-04-13T21:32:13Z

Kokoro seemed to have issues even with my first non-rebased commit though? I can push a dummy commit if you want and see if it resolves anything?

feywind · 2020-04-13T21:54:04Z

Sure, let's try it for science :)

It looks like the checks are passing at this point. Hopefully we'll have our automated sample failure bot going on this repo soon.

feywind · 2020-04-13T21:55:13Z

src/message-queues.ts

    if (deferred) {
      deferred.resolve();
    }
+
+    if (this.numInFlightRequests === 0 && this._onDrain) {


Oh, super minor thing here that I thought @bcoe was going to comment on - to be overly defensive, it might be nice to make this <= 0.

feywind · 2020-04-13T23:13:02Z

Apparently the actual issue with the CI here is that Kokoro is having issues. I'll try to get this merged tomorrow.

Track in-flight requests and add an `onDrain`deferred that resolves when all in-flight requests are done.

Track in-flight requests and add an `onDrain`deferred that resolves when all in-flight requests are done. Co-authored-by: Megan Potter <57276408+feywind@users.noreply.github.com> Co-authored-by: Benjamin E. Coe <bencoe@google.com>

* fix(close): ensure in-flight messages are drained (#952) Track in-flight requests and add an `onDrain`deferred that resolves when all in-flight requests are done. Co-authored-by: Megan Potter <57276408+feywind@users.noreply.github.com> Co-authored-by: Benjamin E. Coe <bencoe@google.com> * chore: update equal to strictEqual for linter changes Co-authored-by: Jeff Hansen <jeffijoe@hotmail.com> Co-authored-by: Benjamin E. Coe <bencoe@google.com>

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Apr 9, 2020

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Apr 9, 2020

jeffijoe force-pushed the master branch from 3e1d4bc to 357af88 Compare April 10, 2020 12:34

feywind reviewed Apr 10, 2020

View reviewed changes

feywind added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 10, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 10, 2020

jeffijoe force-pushed the master branch from 357af88 to 1cde44b Compare April 13, 2020 13:11

jeffijoe force-pushed the master branch from 1cde44b to f8708b0 Compare April 13, 2020 20:27

feywind added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 13, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 13, 2020

feywind approved these changes Apr 13, 2020

View reviewed changes

fix(close): ensure in-flight messages are drained

53bb3b4

Track in-flight requests and add an `onDrain`deferred that resolves when all in-flight requests are done.

jeffijoe force-pushed the master branch from 8e7efc7 to 53bb3b4 Compare April 14, 2020 12:00

feywind added 2 commits April 14, 2020 11:07

Merge branch 'master' into master

4b389ba

Merge branch 'master' into master

2096657

bcoe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 15, 2020

Merge branch 'master' into master

ba015c9

feywind added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Apr 15, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 15, 2020

feywind merged commit 93a2bd7 into googleapis:master Apr 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(close): ensure in-flight messages are drained #952

fix(close): ensure in-flight messages are drained #952

jeffijoe commented Apr 9, 2020 •

edited

googlebot commented Apr 9, 2020

jeffijoe commented Apr 9, 2020

googlebot commented Apr 9, 2020

feywind commented Apr 10, 2020

feywind Apr 10, 2020

jeffijoe Apr 10, 2020

feywind Apr 10, 2020

jeffijoe Apr 10, 2020

feywind Apr 13, 2020

jeffijoe Apr 13, 2020

feywind Apr 13, 2020

jeffijoe commented Apr 13, 2020

feywind commented Apr 13, 2020

feywind commented Apr 13, 2020

jeffijoe commented Apr 13, 2020

feywind commented Apr 13, 2020

feywind Apr 13, 2020

jeffijoe Apr 14, 2020

feywind commented Apr 13, 2020

fix(close): ensure in-flight messages are drained #952

fix(close): ensure in-flight messages are drained #952

Conversation

jeffijoe commented Apr 9, 2020 • edited

googlebot commented Apr 9, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

jeffijoe commented Apr 9, 2020

googlebot commented Apr 9, 2020

feywind commented Apr 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffijoe commented Apr 13, 2020

feywind commented Apr 13, 2020

feywind commented Apr 13, 2020

jeffijoe commented Apr 13, 2020

feywind commented Apr 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feywind commented Apr 13, 2020

jeffijoe commented Apr 9, 2020 •

edited