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: Added connection error timeout #485

Closed
wants to merge 5 commits into from

Conversation

fraserbenjamin
Copy link

@fraserbenjamin fraserbenjamin commented Apr 4, 2024

Resolves #484

Description:
Adds a timeout when a connection to SQS fails due to a lack of internet connection, invalid queue name etc.

Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Why is this change required?:
Currently when a connection fails it will be immediately retried causing an instant loop of retries with lots of unnecessary requests and error events.

Code changes:

This pull request primarily involves renaming "authentication errors" in the codebase, changing them to "connection errors". and adding the additional failure error events. This includes changes in variable names, method parameters, and error handling logic. The changes are spread across the src/consumer.ts, src/types.ts, src/errors.ts, and test/tests/consumer.test.ts files.

Here are the most important changes:

Renaming and refactoring:

  • src/consumer.ts: Renamed the authenticationErrorTimeout variable to connectionErrorTimeout in the Consumer class and updated the corresponding assignment and usage in error handling. [1] [2] [3]
  • src/types.ts: Updated the ConsumerOptions interface to rename authenticationErrorTimeout to connectionErrorTimeout.
  • src/errors.ts: Updated the isConnectionError function to include more error codes that are considered as connection errors.
  • test/tests/consumer.test.ts: Updated the test cases to use CONNECTION_ERROR_TIMEOUT instead of AUTHENTICATION_ERROR_TIMEOUT. Also, added a test case to handle connection errors. [1] [2] [3]

These changes appear to broaden the scope of errors considered as connection issues and handle them uniformly across the application.


Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@fraserbenjamin fraserbenjamin requested review from a team as code owners April 4, 2024 09:08
Copy link

github-actions bot commented Apr 4, 2024

CLA Assistant Lite bot CLA CHECK All Contributors have signed the CLA

@fraserbenjamin
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

@nicholasgriffintn
Copy link
Member

nicholasgriffintn commented Apr 6, 2024

We have a function called isConnectionError here: https://github.com/bbc/sqs-consumer/blob/main/src/errors.ts#L43

That will use the authentication error timeout, would it not be better to extend that and then maybe change the error message to mention authentication/connection?

Signed-off-by: Fraser Benjamin <[email protected]>
Signed-off-by: Fraser Benjamin <[email protected]>
@fraserbenjamin
Copy link
Author

fraserbenjamin commented Apr 6, 2024

We have a function called isConnectionError here: https://github.com/bbc/sqs-consumer/blob/main/src/errors.ts#L43

That will use the authentication error timeout, would it not be better to extend that and then maybe change the error message to mention authentication/connection?

Hey, great suggestion thank you, I've just updated the PR to renamed the timeout and handle the additional errors that come from SQS. If there's anything else you'd like me to change please let me know!

Copy link

codeclimate bot commented Apr 8, 2024

Code Climate has analyzed commit b9554a7 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

The test coverage on the diff in this pull request is 90.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 98.2% (0.0% change).

View more on Code Climate.

@nicholasgriffintn
Copy link
Member

Sorry, I have been delayed somewhat in looking at this, I'm not on this full time.

Also, looks like I might have confused a little bit, the changes here are a bit too much for what's actually needed here, we just need to extend the conditional, we also don't want Connection Error to be true for every error, using SQSError would mean that every error would be treated as a connection error.

I've opened a PR here with the change that I think is needed, I'll close this one in favour of that: #489

@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: When consumer fails to react to SQS instant retry occurs
3 participants