Panic in Simpleworker #2241
Comments
|
Aha... so did you get the message for "performing job..." at line 81 too? If then, it could be an issue with calling the function. There is no If you can see the "performing job..." line correctly, but see panic for the "completed job..." line, it could be another issue. In this case, please let me know what was your job definition and if this issue is easily reproducible. Providing steps for reproducing or sample application could be the best. Lines 80 to 111 in e8274e6 |
|
@sio4, Yes, it throws the same error in both lines. No worries. Thanks for taking the time in solving this :) |
|
I took a look at the issue but I could not reproduce the issue and I feel like there is no reason for the panic on that part. The below is the definition of Lines 14 to 26 in e8274e6 |
|
@sio4, Yes, weirdly, it's throwing a panic in the string. An improvement will be actually checking the error returned by I will try to marshal that Job and see if it panics. It could be an underlying data structure causing this, and it might be a proto issue. |
|
No, what I understand from the error message is that the panic is on executing the So I want to see your code block of the Job definition and how it calls the var j *Job
w.Perform(*j)but maybe not. Right? In this case, I declare the variable For the panic on |
|
By the way, what are your versions? Could you provide the output of |
|
@sio4 , The job isn't a pointer, and the job does execute and fans out the notification as intended. I checked all of the Arguments none of them are nil. If any is nill, then it won't be able to perform the job, and it safely returns |
|
Buffalo Version: |
|
Oh, that is much more interesting. So the job worked without any issue but those two debugging outputs had panic? What is your OS, system architecture, and golang version? also, please post your output of |
|
@sio4, Yes, the job worked fine. That's the weird part. And it's not a data race issue. I thought maybe for some reason it gets fired twice. One with a job and the other Nil, but that wasn't the case. The job ran once. OS: ubuntu 18.04 x64 |
Yeah, I thought similar situation but since we don't have proper logs, cannot prove it. :-( How about adding some application-level logs when calling
By the way, you mention that your go version is 1.18.1, and also tested with 1.17, but from the output of
|
|
@sio4 , So I found the culprit and as expected it was due to the sync.Map in the default_context being nil. We had similar issues in the past if we wanted to access ctx.Value with a string key when the default_context is manually created and not through an HTTP request. |
|
Oh, Got it. So you passed the w.Perform(worker.Job{
Queue: "default",
Handler: "push_notification",
Args: worker.Args{
"context": c,
"r": re.r,
"u": req,
},
})However, if it's the case, the issue is on your usage of the background workers. The argument should be able to be serialized in JSON format, and also it should not be a memory reference (should be a kind of call by value). The reason is simple. The concept of the background worker is "distributed long-running task execution on the other machine" even though the Simple worker itself is designed to run the job in the same process in a new goroutine. Currently, your job works fine since the memories you passed to the worker are still accessible by goroutine in the same process. However, if you extend your service and want to run those long-running tasks on separated machines, and want to replace the worker with another worker that supports remote queueing via message queues such as https://github.com/gobuffalo/gocraft-work-adapter, https://github.com/stanislas-m/amqp-work-adapter, or whatever, they will not work since your arguments cannot be serialized as a JSON message which is needed to be passed to the other machine via a message queue. (you should be free to replace any worker adapter implementation without code change which calls For flexibility, the Lines 5 to 11 in e8274e6 So the solution is that you should collect which information is needed for the job (e.g. user id, message to the user, or database key for the additional information), add those values to your own Arg structure, make sure the Arg is able to be marshaled as JSON safely, then pass the Arg to the job. I think I need to consider improving our document for the part, and also try to find a way to verify the arguments programmatically too. Can it be the answer? |
|
Anyway, even though the usage of background workers should be improved to make it compatible with the fundamental concept of it, still the bug you found ( |
|
@sio4 , apologies for the late response. Yes, I agree better documentation would be needed for the purpose and parameters of how to use the simple worker model. Yes, thanks for pushing the PR. I confirm that the PR fixed the issue. I agree with your suggestion on improving the usage of the simple worker model on our end and we will apply those changes. |
|
Fixed in PR#2246 |
|
No problem! We all live in our own time zone, no need to saying sorry for the very small latency :-)
Happy to hear that! Have a good weekend! |
Hello,
I'm getting panic in Debug logger when a simple worker completes the job
That's the culprit and this code panics too
The text was updated successfully, but these errors were encountered: