-
Notifications
You must be signed in to change notification settings - Fork 28
Feat: Add Abort Signal Implementation #70
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
base: master
Are you sure you want to change the base?
Conversation
|
Could you describe the scenario that requires using the Abort API? Some pseudocode could be helpful, Is it possible to end the retry by returning |
Maybe react useEffect? |
|
My use case is for a polling script that has a graceful shutdown function where receiving a SIGTERM will start cleaning up and sending a signal to abort. The graceful shutdown also has a timeout to exit if everything isn't cleaned up by then. For a more specific scenario, the built-in fetch API takes in an Abort signal to stop the http request. Normally this wouldn't be an issue, but if you wrap this retry library around the fetch call, this library will go through all the retries, which may be lengthy. If the library can handle the same signal that was passed to the fetch API, then the library could also abort the retrying. |
AiSocialNinjaLabs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I need this same commit for my application?
Caution
working with large data sometimes needs these kinds of fixes in order to maintain repository health.
@AiSocialNinjaLabs, reference #70 @heyppen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been on my developers 👨🏻💻 quest for some time now. I have no formal training or education🏛️. I never graduated high school💸, the reason for that is they never had anything I found valuable for my future unfortunately, it wasn’t for lack of trying.and the only reason I had gotten my GED was to join the military(“Best decision of my life to be of service to my country! Hoah!”💯) Anyways, not that any of this really matters to most of you reading this, but, for the one special one like me… Like us… This is for you! I’m just feeling some kind of way right now and I wanted to leave this for any other intuit_dev.soldiers out there that might “accidently”😁, stumble across this… I am self taught on everything. So don’t give up! Your intuition will set you free if you can believe and trust yourself. It’s a lot harder than people will ever know. I am the Ninja-man™🥷
and this is the GhostWriters declaration of pointlessness to those that don’t really matter and as a matter of fact it is for the ones who really do matter, to anyone who actually gives a f@$#! out there! Lastly I GOT MAD LOVE AND RESPECT FOR ALL OPEN SOURCE AND ESPECIALLY GITHUB! THANK YOU AND GOod evening… Social Ninja vanish!🥷😶🌫️?
| } | ||
|
|
||
| throw new Error("Something went wrong."); | ||
| throw new Error(this.isAborted ? "Retry aborted" : "Something went wrong."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my only gripe is not using signal.reason if available. That would be a bit of a refactor here, but I don't think we'd want to NOT use one of the built in values of the AbortController.
I think its reasonable that the reason would populate into the error here to be handled, as well as a separate handling of that inside the catch
| const shouldRetry = await this.options.retry(e, this.attemptNumber); | ||
|
|
||
| if (!shouldRetry || this.attemptLimitReached) { | ||
| if (!shouldRetry || this.isAborted || this.attemptLimitReached) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not correct to do an abort in between executions - same behavior could be achived in user-defined retry method. Instead setTimeout located here, should be cleared.
|
Happy to pick up addressing the code review if @developertom01 has moved on to other things in the last year. |
AbortSignalto terminate retry logic.AbortSignalto the retry logic. This will allow the user to cancel the retry logic if needed.AbortConrollerin typescript types, I bumped@types/nodeto22.4.1version