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

fixed panics on msg.Name != "" & redis is disabled #106

Open
wants to merge 1 commit into
base: v3
Choose a base branch
from

Conversation

mvisonneau
Copy link

When using the tool without redis, if the Name of a taskq.Message is set, we end up with the following error:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x48 pc=0xa3dcac]

goroutine 1 [running]:
github.com/vmihailenco/taskq/v3.redisStorage.Exists(0x0, 0x0, 0xf1bf30, 0xc00002a0b8, 0xc0005fe120, 0x13, 0x13)
        /home/max/go/pkg/mod/github.com/vmihailenco/taskq/v3@v3.2.3/taskq.go:66 +0x4c
github.com/vmihailenco/taskq/v3/memqueue.(*Queue).isDuplicate(0xc00059e980, 0xc000580b60, 0x0)
        /home/max/go/pkg/mod/github.com/vmihailenco/taskq/v3@v3.2.3/memqueue/queue.go:263 +0x8e
github.com/vmihailenco/taskq/v3/memqueue.(*Queue).Add(0xc00059e980, 0xc000580b60, 0x12, 0xc00059ab70)
        /home/max/go/pkg/mod/github.com/vmihailenco/taskq/v3@v3.2.3/memqueue/queue.go:179 +0x5

@vmihailenco
Copy link
Owner

I think I prefer to see an error when storage is not configured. I am not sure that local storage is a good default choice (+ it adds extra dependency).

You can still set opt.Storage = localStorage{} in your app when configuring a queue...

@shaunco
Copy link
Contributor

shaunco commented Nov 20, 2021

You can still set opt.Storage = localStorage{} in your app when configuring a queue...

Only if localStorage becomes LocalStorage or a NewLocalStorage() function is added... otherwise localStorage is only usable internal to taskq.

...

As a user of taskq via memqueue instead of Redis, this finally allows us to name tasks and prevent duplicates. That said, I find it very strange that named tasks will always Exists once added to a queue when it seems like they should stop existing once successfully executed... but Storage only has a single Exists method, which means Consumer has no way to remove a successfully executed task from storage.

@shaunco
Copy link
Contributor

shaunco commented Nov 26, 2021

Seems like this can be closed since #162 was accepted. Might be worth noting in the readme opt.Storage = taskq.NewLocalStorage() should be set to make use of names.

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.

3 participants