-
Notifications
You must be signed in to change notification settings - Fork 0
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
The road to v0.1
🚀
#15
Comments
The manager does not respect cancelation of the context. Is this intentional behavior? I would expect that when the context passed to the |
I would add a constructor for the |
Note regarding the documentation: often times the word |
The manager is prone to race condition: Example is calling the following consecutive calls:
Also, I guess I shouldn't be able to call Close twice, neither should I be able to run tasks after closing the manager? |
I would remove |
|
How do I stop the task if the task is stalled? I can imagine that one of the use cases for the |
@CermakM thank you for taking the time to look at the codebase and provide feedback. ❤️ Below you will find my responses to your review notes.
Very much intentional. Usually you run a background task from the context of a request, and you definitely want the background task to continue running even when the request has been processed and response delivered.
Not sure where exactly the race condition happens here, please explain. 🙏 Generally, if you schedule one-off tasks after calling
I actually think you should be able to do that and the result should always be the same. However, it is somewhat pointless as usually you want to call I guess you could end up in a weird situation where you call
I thought about this as well, I went ctor-less way because there was nothing needed to be done in them. Perhaps I can add them to future-proof the API. 🤔
If I move it to
You cannot at the moment. The only way out is to let the task run to completion - the
|
Thanks for the answers! I'll move some of the points to their respective issues to make the conversation easier, if you don't mind. |
True that, fair. I would move it to the background.go then 👍 |
Also, regarding future proofing, I would also add |
Also a nitpick here (but I believe useful in general). It's a good practice to define the errors where they are being used. For example here: strv-backend-go-background/task/task.go Line 23 in 1ff5d9f
This error is defined in the |
I would also rename |
I would just add here, that v0.1 is already released, so I would close this issue and create new milestone v0.2, where new issues can be addresed. |
Use this issue to leave feedback as to what you think needs to be done before publishing our very first v0.1 release of
go.strv.io/background
package.The release is planned for Monday, 18th of March but it might be postponed if needed, based on the received feedback.
The text was updated successfully, but these errors were encountered: