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

Panic when creating jet stream consumer on wildcard stream #1641

Open
areknoster opened this issue Jun 5, 2024 · 8 comments
Open

Panic when creating jet stream consumer on wildcard stream #1641

areknoster opened this issue Jun 5, 2024 · 8 comments
Labels
defect Suspected defect such as a bug or regression

Comments

@areknoster
Copy link

Observed behavior

When creating consumer against embedded NATS server JetStream with wildcard (>) subscription, the function panics.

Expected behavior

The function doesn't panic and returns consumer that's usable.

Server and client version

github.com/nats-io/nats-server/v2 v2.10.11
github.com/nats-io/nats.go v1.35.0

Host environment

GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOVERSION='go1.22.3'

Steps to reproduce

Run this test: #1640

@areknoster areknoster added the defect Suspected defect such as a bug or regression label Jun 5, 2024
@piotrpio
Copy link
Collaborator

piotrpio commented Jun 6, 2024

Hello @areknoster, thanks for reporting the issue and thank you for creating the test - it looks like we're getting an unexpected response type from the server for create consumer request. I'll take a look and let you know when I have a fix.

@ripienaar
Copy link
Contributor

ripienaar commented Jun 6, 2024

The client should not allow a wildcard stream with publish ack set it will break everything.

@piotrpio
Copy link
Collaborator

piotrpio commented Jun 6, 2024

The issue here is that if you create a stream with > in subjects, it means it will also be sending acks for all messages published on >, even if it's the JetStream API itself (so e.g. subject used to create the consumer). If you wish to have a catch all stream, it should be configured with NoAck: true option in stream config.

I'll be adding a validation in the client to disallow creating such streams.

@areknoster
Copy link
Author

Thank you for the responses!
Now I understand the underlying issue. The usecase I had in mind is a setup, where all async communication would be stored in a single stream. Now I see, that this would not ge desirable. It would also catch all internal JetStream communication. Coming from other pub/sub solutions it's not obvious though. Splitting it up like command.>, event.> would fix the issue.

I have additional thoughts on the issue:

  1. Why should the validation against > stream happen in the client, not server?
  2. Some documentation one the issue would be useful - I'd be happy to contribute
  3. Unexpected response of any sort should not cause a panic, even if it's unexpected - once again, I'd be happy to contribute to fix that

@derekcollison
Copy link
Member

@piotrpio I do not want us to disallow broader scoped subjects for streams but as you mention we should suppress pub acks if the subject is sufficiently wide. The server needs to support this of course and enforce the no pub ack config on those wider stream subjects.

@derekcollison
Copy link
Member

@areknoster You are correct for certain subjects we should have the server perform the check.

@piotrpio
Copy link
Collaborator

piotrpio commented Jun 6, 2024

@derekcollison Agreed, it would be good for the server to perform the validation.

@areknoster I agree that the client should not panic. We'll have to add a check for nil response in each JS API call. I will create an issue for that and if you wish to contribute that would be great!

@piotrpio
Copy link
Collaborator

piotrpio commented Jun 6, 2024

@areknoster I've created the issue: #1642

If you wish to contribute, please assign yourself to it - thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect Suspected defect such as a bug or regression
Projects
None yet
Development

No branches or pull requests

4 participants