Comments (17)
-
the idea from
net/http
seems good, but the functions nameNewServer
is confused.If i did not read the doc, i would think it's a server to receive request and produce a job. other names likeServe
,NewServeMux
also confused -
Handler is part of Config
: i prefer the api likegin
bg := asynq.NewBackgroud(...)
bg.Use(middleware)
bg.Register(handler1)
bg.Register(handler2)
from asynq.
I see. I can see why this could be confusing. Let me try to explain.
First Question
since we have Stop method, what does Shutdown do ? this method make me think the process will exist.
Asynq took a lot of ideas from Sidekiq and they use OS signals to handle graceful shutdown (video).
Currently, when you start background workers with bg.Run
it waits for signals.
Asynq handles TERM, INT, and TSTP signals.
If you send TSTP signal to the process, asynq will stop processing new tasks but doesn't exit. This is documented in Sidekiq here.
If you send TERM or INT signal to the process, asynq will wait for few seconds to let active workers finish its tasks but if they don't finish within that timeout, asynq will push the tasks back to Redis and exit the process. This is documented in Sidekiq here.
My intention was to name the first operation Stop
and the second one Shutdown
. But I can see why Stop
is a bit confusing. Looking at sidekiq doc, maybe we can name it bg.Quiet()
?
Does that makes sense?
Second Question
bg.Handler can receive options to custom each task handler, i think its important. i can't find this feature in current design, the option like Queues and Concurrency seems should be task level instead of global level
I agree with the Concurrency
per Handler (i.e. Concurrency
per task type). For example, if you have a task to use third-party API and if they have a limit on how many concurrent API calls they can handle, this is definitely useful. Am I understanding you correctly here?
I'm not sure if we can add an option to specify Queues per Handler though. gocraft/work
has a different design in that they have one-to-one mapping between job-type and queue. Asynq's design is similar to sidekiq. It allows for multipe queues with different priority but any task can be put on any queue. So I'm not sure if we can create Queue option for a Handler. (please let me know if I'm not understanding your point correctly).
from asynq.
Yep, just exporting just Start
and Stop
makes sense to me (chose Stop
because of the symmetry with Start
).
So effectively, if user uses Start
and Stop
instead of Run
they don't get some nice signal handling but I guess that's okay. If we get feature request to export Quiet
(i.e. stop processing new tasks), maybe we'll add that later.
I'll get started on making these changes and ping you in the PR π
from asynq.
It's fine. breaking change needs to thinking more and more.
Looking forward to your final design.
from asynq.
+1 to exporting these. Some weeks ago I forked and exposed start/stop myself but have now stumbled on this thread and would be happy to remove my fork and use this once itβs ready!
My current implementation (importing my fork) handles the syscall termination signals for the workers as I was already handling them for other things before using asynq and itβs helpful to me to have my graceful shutdown logic collocated. It was also trivial to implement a shutdown timeout external to this library.
Thanks for your work on this very usable project, looking forward to seeing these updates!
from asynq.
@zsiec Awesome to hear your feedback! Thank you.
I'm currently drafting a PR for this change, which includes a few breaking changes (I'll make sure to document the upgrade paths in CHANGELOG for those who are on the current version).
from asynq.
Closed via #135
from asynq.
@yxlimo Thank you for opening this issue!
Would exporting Background.Start
and Background.Stop
would resolve this?
Proposal
-
Keep
Background.Run
as is, since it also does signal handling. -
Expose
Background.Start
andBackground.Stop
, if a user of the package wants to have more control over when to start and stop the background workers .
Please let me know your thoughts and feedback!
from asynq.
Looks good.
also export Background.WaitSignal
? more control and more convenient
from asynq.
Yep, I agree.
So if you opt out of using Run
then code would look something like this to achieve the same effect:
bg := asynq.NewBackground(redis, asynq.Config{
Concurrency: 20,
})
bg.Start(handler)
bg.WaitForSignals()
// Do any cleanup here if needed.
bg.Stop()
would that work for your use case?
from asynq.
its work, thanks :)
from asynq.
Actually I've been thinking about doing some major API changes and I think this is a great opportunity to clean up some things.
Let me run this by you and hopefully get your feedback.
func main() {
r := asynq.RedisClientOpt{
Addr: ":6379",
}
mux := asynq.NewServeMux()
mux.HandleFunc(tasks.EmailTask, tasks.HandleEmailTask)
// Renamed the type to more common name "Server"
srv, err := asynq.NewServer(asynq.Config{
RedisConnOpt: r,
Handler: mux, // <- Handler is part of Config
Concurrency: 20,
Queues: map[string]int{
"critical": 6,
"default": 3,
"low": 1,
},
})
if err != nil {
log.Fatal(err)
}
srv.Run()
}
Or optionally you can do Serve
and Shutdown
to start/stop background workers manually.
I've take an API ideas from net/http
's Server
type.
func main() {
r := asynq.RedisClientOpt{
Addr: ":6379",
}
mux := asynq.NewServeMux()
mux.HandleFunc(tasks.EmailTask, tasks.HandleEmailTask)
srv, err := asynq.NewServer(asynq.Config{
RedisConnOpt: r,
Handler: mux, // <- Handler is part of Config
Concurrency: 20,
Queues: map[string]int{
"critical": 6,
"default": 3,
"low": 1,
},
})
if err != nil {
log.Fatal(err)
}
done := make(chan struct{})
go func() {
sigint := make(chan os.Signal, 1)
signal.Notify(sigint, os.Interrupt)
<-sigint
// We received an interrupt signal, shut down.
if err := srv.Shutdown(context.Background()); err != nil {
// Error from workers shutdown, or context timeout:
log.Printf("background workers Shutdown error: %v", err)
}
close(done)
}()
if err := srv.Serve(); err != asynq.ErrServerClosed {
log.Fatal("background workers Run error: %v", err)
}
<-done
}
Since this is a disruptive but important change, any feedback is welcome π
from asynq.
Great feedback, thank you! Let me think about this a bit more.
from asynq.
Ok here's my next proposal, feedback appreciated!
Add
func (bg *Background) Use( ...Middleware)
func (bg *Background) Handle(string, Handler)
func (bg *Background) HandleFunc(string, HandlerFunc)
func (bg *Background) Start() error
func (bg *Background) Stop()
func (bg *Background) Shutdown()
ErrBackgroundClosed
: onceStop
orShutdown
is called on aBackground
, this error will be returned fromRun
orStart
to prevent reuse.
Remove
ServeMux
type (it's internalized inBackground
)
Change
func (bg *Background) Run(Handler)
tofunc (bg *Background) Run() error
Example
After the change, main
may look something like this.
func main() {
bg := asynq.NewBackground(redis, asynq.Config{ ... })
bg.Use(loggingMiddleware)
bg.Handle(task.EmailTask, tasks.EmailTaskHandler)
bg.Handle(task.ImageProcessing, task.ImageProcessingHandler)
// bg.HandleFunc(type, fn) for Handler functions
bg.Run()
}
Or bg.Run()
could be replaced with
done := make(chan struct{})
go func() {
sigs := make(chan os.Signal, 1)
signal.Notify(sigs, unix.SIGTERM, unix.SIGINT, unix.SIGTSTP)
for {
s := <-sigs
if s == unix.SIGTSTP {
bg.Stop() // Stop processing new tasks
continue
}
break
}
// We received an interrupt or term signal, shut down.
bg.Shutdown()
close(done)
}()
if err := bg.Start(); err != asynq.ErrBackgroundClosed {
log.Fatal("Background Start error: %v", err)
}
<-done
from asynq.
awesome! just a little confused.
- since we have
Stop
method, what doesShutdown
do ? this method make me think the process will exist.
and here is more thinking(based on the experience of using gocraft/work
and sidekiq
):
bg.Handler
can receive options to custom each task handler, i think its important. i can't find this feature in current design, the option likeQueues
andConcurrency
seems should be task level instead of global level
from asynq.
totally right. thanks for your explanation.
now i understand the design for Queues
For the first question
after reading the wiki, i found that TSTP just only stop processing new task, and TERM is more useful.
in go
language, i found most packages have only one Stop
method(or Close
).i think it's enough. so stop
method do not need to be exported.
func (bg *Background) Close() {
bg.stopProcessing()
// handle timeout
}
leave two methods maybe will let user like me confused too, we can't assume that everyone will read the doc. What do you think?
from asynq.
@yxlimo Now that I've started working on these API changes. I have a few thoughts and want to think about them more (sorry, for going back and forth so many times π).
If this change is blocking you, please feel free to fork and use that in the meantime!
from asynq.
Related Issues (20)
- [BUG] The related methods of Inspector basically lack the transmission of ctx HOT 2
- [FEATURE REQUEST] customize serializer
- Discussion/Suggestion: Decoupling `x` and `tools` packages.
- [BUG] Unable to schedule a task HOT 1
- Timing issues when running certain tests
- [FEATURE REQUEST] Settable maxArchiveSize HOT 1
- [BUG] Delete archived task fail, when task has been evicted from archived zset. HOT 2
- [FEATURE REQUEST] Unique option for run time of task
- [FEATURE REQUEST] aggregation task optimization
- handler not found for task "ping:worker"
- [FEATURE REQUEST] Custom ticker value for heartbeater duration HOT 4
- Scheduler: How to get the schedule time of the task in the task handler? HOT 1
- [FEATURE REQUEST] Exposing MinIdleConns redis config HOT 3
- [BUG] Description of the bug HOT 1
- [FEATURE REQUEST] can add opt for cron to set second HOT 2
- [FEATURE REQUEST] Process task with ratelimit HOT 2
- [BUG] After manually deleted redis keys, some of the features like metrics, inspector aren't working properly
- [FEATURE REQUEST] Wrap the payload contents in the CLI's Task Info modal
- [FEATURE REQUEST] How can I specific worker concurrency for each queue
- [FEATURE REQUEST] pause server
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
π Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. πππ
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google β€οΈ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from asynq.