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

first implementation #1

Merged
merged 5 commits into from
Nov 16, 2022
Merged

first implementation #1

merged 5 commits into from
Nov 16, 2022

Conversation

KeiichiHirobe
Copy link
Contributor

@KeiichiHirobe KeiichiHirobe commented Nov 15, 2022

What I did

Please read README and test codes to understand features and interfaces.
Feel free to ask questions if you have.

I found a bug after submitting this PR, I'll fix in a new PR!
see: #2

Next Action

I'll submit as new PRs for features below soon.

Copy link

@ouchi2501 ouchi2501 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still reviewing, but once I commented
I have not been able to review all the codes, so please wait a little longer!

go.mod Show resolved Hide resolved
go.mod Outdated
@@ -0,0 +1,5 @@
module github.com/go-misc/async-retry

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nits]

How about github.com/Kyash/async-retry as a module name?
I don't have a strong opinion, so I'll leave it to you to decide if you want to modify it or not.

Copy link
Contributor Author

@KeiichiHirobe KeiichiHirobe Nov 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a trash made when I developed at local environment. Thanks!
cf84aa6

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix!
Please also fix the sample code listed in the README

https://github.com/Kyash/async-retry/blob/first-impl/README.md?plain=1#L26

options.go Show resolved Hide resolved
Copy link

@ouchi2501 ouchi2501 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented.
Review is complete

async_retry.go Show resolved Hide resolved
async_retry.go Show resolved Hide resolved
Copy link

@ouchi2501 ouchi2501 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@KeiichiHirobe KeiichiHirobe merged commit b56b220 into main Nov 16, 2022
@KeiichiHirobe KeiichiHirobe deleted the first-impl branch November 16, 2022 08:01
a.mu.Lock()
select {
case <-a.shutdownChan:
return InShutdownErr
Copy link
Contributor

@convto convto Nov 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mu.Unlock() is not called when a.shutdownChan is already closed.
This means that if a asyncRetry.Do is called twice on asyncRetry resource that has already been shutdown, the second call will fail to acquire a mu.Lock() and will result in a deadlock.
I think it should be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx!
You are right!
I've just submitted for this bug: #4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

shutdown is not always safe
4 participants