Multiple polls invoked

[UMFsimke]

Based on a code from master branch:

public start(): void {
    if (this.stopped) {
      debug('Starting consumer');
      this.stopped = false;
      this.poll();
    }
  }

  public stop(): void {
    debug('Stopping consumer');
    this.stopped = true;
  }

private poll(): void {
    if (this.stopped) {
      this.emit('stopped');
      return;
    }

    /* tldr */
      }).then(() => {
        setTimeout(this.poll, currentPollingTimeout);
      }).catch((err) => {
        this.emit('error', err);
      });
  }

If I would do something like:

consumer.start();
consumer.stop();
consumer.start();

Wouldn't this actually invoke two times:

this.receiveMessage(receiveParams)
      .then(this.handleSqsResponse)

Which would essentially lead toward double polling and quota usage? Shouldn't there be some usage of clearTimeout() method in stop() if polling is already scheduled via setTimeout() so that we interrupt poll() from being invoked?

I was thinking to use this approach where I would stop a consumer when the queue is empty and in certain cases, if it's not running to start polling again, although I believe that this could be an edge-case and a bug.

[jwalton]

Also, if you call:

consumer.start();
consumer.stop();

you would expect your handler would not ever be called, since you have now stopped the consumer, but in fact the consumer is currently waiting for a message, and if a message is received our message handler will be called.

[nicholasgriffintn]

It shouldn't actually get to recieveMessage to if you immediately called stop after start note, as Poll has this at the top:

private poll(): void {
    if (this.stopped) {
      this.emit('stopped');
      return;
    }

(validated in this test: https://github.com/bbc/sqs-consumer/blob/main/test/consumer.test.ts#L986, so this should not be happening in this case)

It may call recieveMessage multiple times if you did start, stop, start in succession.

Need to validate if that is actually happening or not, as we should focus on testing that before a solution if it is.

[jwalton]

@nicholasgriffintn When you call start(), it calls into poll() immediately and synchronously, so this.stopped will be false when we call into poll(). That test case validates that handleMessage was called once, and it should be called not at all.

Even if the handler were not called again, there is other undesirable behavior from the current state of affairs. If you write a program that calls start() and then stop(), the program will take 20 seconds to run, even though it isn't doing anything, because it will wait for that open network connection to timeout. Ideally calling stop() should abort the current poll request, which would prevent us from receiving further messages, and would clean up the requests and let the process exit gracefully.

[nicholasgriffintn]

Hmm not sure about it not being called at all, I think if you call start(), you would want it to poll immediately, so I would say that is expected functionality. What is not expected, is that if that the timeout passes and then it receives messages again, which it does not do in the case where you immediately call stop.

If you call start and then stop and then start, it will receive messages multiple times as the timeout has not been cancelled, but in my local tests of the suggested PR, this does not appear to have been fixed, and this appears to be what this issue is about from my understanding.

[nicholasgriffintn]

I would say an abort would probably be different functionality though, ie: you would call a new function abort rather than stop as sqs-consumer is used in the case where it is expected that it is still running the original poll when stopped, but runs no more after.

[jwalton]

where it is expected that it is still running the original poll when stopped, but runs no more after

Sorry, but this is not at all my expectation of a function called “stop”. When I close a listening socket, I don’t want any more connections from it. When I remove a listener from a dom element, I don’t want any more events from it. When I stop an SQS listener, it should be likewise.

The most common use case for a function like stop() is when your server is shutting down and you want to do so cleanly. For example, when you upgrade a pod in kubernetes, your process will get a sigterm, and you shut down your listening port so you won’t get any more http requests, and you call stop() on your sqs listener so you won’t get any more sqs messages. Then your process finishes up any outstanding http requests or messages it was working on and shuts down, so another pod can take its place. The current stop() waits between 0 and 20 seconds, and adds another message to the queue which further slows down the shut down time of the pod.

What is your use case for “stop… in a little bit and maybe get me one more message”?

[nicholasgriffintn]

What you are explaining is what stop does in sqs-consumer, start imitates a poll which calls ReceiveMessage, this will wait for the length of time configured in the option waitTimeSeconds, which defaults to 20 seconds (you can also set this to 0 to opt out of long-polling).

This request is outstanding and sqs-consumer is finishing up a process that has already started, no more connections/ api calls will be made once this process has concluded.

That's how a lot of implementations expect this library to work. What you are suggesting is to abort the request that was previously made, which would be a different use case.

Again, outside of what this issue is about, which is where polling is triggered multiple times if you start, stop, start, which the PR #344 will resolve.

Aborting will be treated as a separate issue if you would like to raise one.

[rogerweb]

Thanks @nicholasgriffintn for actively working on this project.

In regard to the stop behavior, I have the same understanding as @jwalton and the same pain. I'm able to perform a graceful shutdown within a second for all my dependencies except by the SQS consumer as it is waiting the 20 seconds in the worst case scenario. The way I see it, the pending request to SQS is a sqs-consumer implementation concern, while the exposed functionality would be "please shutdown ASAP, I'm not expecting any new messages, no matter what you are doing internally". If an abort() fits this use case better, I would be very happy with it too.

[nicholasgriffintn]

Thanks I am acknowledging / considering your thoughts here, I must also consider existing implementations is what I'm saying (in particular the BBC's), I've raised a new issue here:

#346

The idea would be to implement this in a way that is backwards compatible by providing a new abort option, like so:

consumer.stop({abort: true})

This will default to false initially, and be changed in the future should we choose to make this to default behaviour.