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

Unclear panic when using Client.Subscribe() when client is not executing jobs #596

Closed
arp242 opened this issue Sep 17, 2024 · 3 comments · Fixed by #599
Closed

Unclear panic when using Client.Subscribe() when client is not executing jobs #596

arp242 opened this issue Sep 17, 2024 · 3 comments · Fixed by #599

Comments

@arp242
Copy link
Contributor

arp242 commented Sep 17, 2024

If I try to subscribe for job updates:

rc, err := river.NewClient(driver, &river.Config{})
if err != nil {
    panic(err)
}
rc.Subscribe(river.EventKindJobCancelled, river.EventKindJobCompleted, river.EventKindJobFailed)

I get a panic:

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

goroutine 1 [running]:
github.com/riverqueue/river.(*subscriptionManager).SubscribeConfig(0x0, 0xc00038d5a0)
        /home/martin/.cache/go/pkg/mod/github.com/riverqueue/river@v0.11.3/subscription_manager.go:223 +0x100
github.com/riverqueue/river.(*Client[...]).SubscribeConfig(...)
        /home/martin/.cache/go/pkg/mod/github.com/riverqueue/river@v0.11.3/client.go:908
github.com/riverqueue/river.(*Client[...]).Subscribe(0xc0001b6020?, {0xc00038d670?, 0x1?, 0x1?})
        /home/martin/.cache/go/pkg/mod/github.com/riverqueue/river@v0.11.3/client.go:881 +0x47

The reason for this is that in river.NewClient() it does (abdriged):

// There are a number of internal components that are only needed/desired if
// we're actually going to be working jobs (as opposed to just enqueueing
// them):
if config.willExecuteJobs() {
	// ...
	client.subscriptionManager = newSubscriptionManager(archetype, nil)
	// ...
}

Which is reasonable and fair enough. I would expect a nicer message than a panic and having to read the source code to figure out why I get a panic. There isn't any error return, but just panic() with a nicer message is enough (this is clearly a programmer error, so it's probably an appropriate usage of panic).

@arp242 arp242 changed the title Unclear panic when using Client.Subscribe() while client is not executing jobs Unclear panic when using Client.Subscribe() when client is not executing jobs Sep 17, 2024
@arp242
Copy link
Contributor Author

arp242 commented Sep 17, 2024

Actually, might be better to make it work, if at all possible, rather than just clarifying the error?

What I have a is a little CLI to manually schedule some jobs and then wait for them to be completed. This is useful sometimes.

But I don't want this to start processing jobs; I just want it to wait until it's done. That is: wait for a worker to pick it up and run it.

Unless I'm missing something, I think this currently isn't really possible (other than periodically calling JobGet(), which is what I'm doing now).

@brandur
Copy link
Contributor

brandur commented Sep 17, 2024

Yeah, the error should be better, but what you're trying to do isn't going to work. A subscription isn't polling the database looking for completed jobs — rather, it's just sending back jobs that this particular client has worked.

From the docs:

Events are only distributed for jobs worked by the specific client that was subscribed to. When running multiple clients across multiple nodes, it's necessary to subscribe to all of them to receive events for all jobs in the entirety of the cluster.

@arp242
Copy link
Contributor Author

arp242 commented Sep 17, 2024

Right, yeah. that's fine – polling isn't too bad.

brandur added a commit that referenced this issue Sep 18, 2024
…t work

Related to #596. If `Subscribe` was called on a client that didn't have
a `Workers` bundle configure a nil pointer panic would occur because
`subscriptionManager` was never initialized.

Here, leave that as a panic since it makes sense to warn a user about an
API misuse that'd undoubtedly lead to more confusion/pain, but improve
the error message so that it's more obvious to the caller why this is a
problem.

Fixes #596.
brandur added a commit that referenced this issue Sep 18, 2024
…t work

Related to #596. If `Subscribe` was called on a client that didn't have
a `Workers` bundle configure a nil pointer panic would occur because
`subscriptionManager` was never initialized.

Here, leave that as a panic since it makes sense to warn a user about an
API misuse that'd undoubtedly lead to more confusion/pain, but improve
the error message so that it's more obvious to the caller why this is a
problem.

Fixes #596.
brandur added a commit that referenced this issue Sep 18, 2024
…t work (#599)

Related to #596. If `Subscribe` was called on a client that didn't have
a `Workers` bundle configure a nil pointer panic would occur because
`subscriptionManager` was never initialized.

Here, leave that as a panic since it makes sense to warn a user about an
API misuse that'd undoubtedly lead to more confusion/pain, but improve
the error message so that it's more obvious to the caller why this is a
problem.

Fixes #596.
tigrato pushed a commit to gravitational/river that referenced this issue Dec 18, 2024
…t work (riverqueue#599)

Related to riverqueue#596. If `Subscribe` was called on a client that didn't have
a `Workers` bundle configure a nil pointer panic would occur because
`subscriptionManager` was never initialized.

Here, leave that as a panic since it makes sense to warn a user about an
API misuse that'd undoubtedly lead to more confusion/pain, but improve
the error message so that it's more obvious to the caller why this is a
problem.

Fixes riverqueue#596.
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 a pull request may close this issue.

2 participants