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

JS: Library possibly removes a consumer when it should not #776

Closed
kozlovic opened this issue Jul 13, 2021 · 5 comments · Fixed by #794
Closed

JS: Library possibly removes a consumer when it should not #776

kozlovic opened this issue Jul 13, 2021 · 5 comments · Fixed by #794
Labels
bug Confirmed reproducible bug

Comments

@kozlovic
Copy link
Member

I think it is dangerous to delete (js.DeleteConsumer) a consumer when user calls sub.Unsubscribe(). Users may be used to call this function as a good practice when being done with a subscription.

The issue here is that if a user subscribes to a consumer that it does not "own" (meaning that the library did not create it), then Unsubscribe() will delete it:

nats.go/js.go

Line 928 in c7fc3c7

if drainMode && (jsi.durable || jsi.attached) {

I believe that the library should ONLY delete JS consumer if it has created it for an ephemeral consumer (called AddConsumer inside js.subscribe()) and that there is a guarantee that no other subscription is using it (so not queue sub, or when AddConsumer returned an error meaning that someone else called AddConsumer()).

@kozlovic kozlovic added the bug Confirmed reproducible bug label Jul 13, 2021
@aricart
Copy link
Member

aricart commented Jul 14, 2021

I would propose that magic subscribe be limited to ephemerals. Durables are important contract for applications that need to be explicitly created and lifecycled.

@ripienaar
Copy link
Contributor

I think someone should take a stab of documenting the overall behavior of subscribe in a adr :)

@philkuz
Copy link

philkuz commented Jul 20, 2021

Do you recommend any work arounds? Running into this issue after converting old QueueSubscribe durable STAN Channel to a corresponding JetStream durable QueueSubscribe. Unsubscribing kills the durable consumer (defeating the purpose of Durability). Not Unsubscribing seems like it will leak state in NATS.

@wallyqs
Copy link
Member

wallyqs commented Jul 20, 2021

@philkuz in current nats.go client you can use Drain() instead to detach, but we will remove the current behavior of deleting the consumer as well

@philkuz
Copy link

philkuz commented Jul 20, 2021

will try that out. Thanks @wallyqs !

kozlovic added a commit that referenced this issue Aug 10, 2021
- Some refactoring to prevent lock inversion (no connection publish
should be done under the subscription lock).
- Remove used of jsSub mutex since so far everything can be done
under the protection of the subscription's lock.
- Attempt to delete JS consumer on Unsubscribe *only* if the library
called AddConsumer and got a success.

Resolves #775
Resolves #776

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
kozlovic added a commit that referenced this issue Aug 15, 2021
They will be described in the release notes, but gist:

Added:
- `DeliverSubject()` option to configure the deliver subject of a JetStream consumer created by the `js.Subscribe()` call (and variants)
- `BindDeliverSubject()` option to subscribe directly to a JetStream consumer deliver subject (bypassing any lookup or JetStream consumer creation)
- Fields `DeliverGroup` in `ConsumerConfig`, `PushBound` in `ConsumerInfo`. They help making prevent incorrect subscriptions to JetStream consumers
- Field `Last` in `SequencePair`

Changed:
- With a `PullSubscription`, calling `NextMsg()` or `NextMsgWithContext()` will now return `ErrTypeSubscription`. You must use the `Fetch()` API
- If the library created internally a JetStream consumer, the consumer will be deleted on `Unsubscribe()` or when the `Drain()` completes
- Fail multiple instances of a subscription on the same durable push consumer (only one active at a time). Also, consumers now have the concept of `DeliverGroup`, which is the queue group name they are created for. Only queue member from the same group can attach to this consumer, and a non queue subscription cannot attach to it. Note that this requires server v2.3.5
- Attempting to create a queue subscription with a consumer configuration that has idle heartbeats and/or flow control will now result in an error

Fixed:
- Possible lock inversion
- JetStream consumers could be incorrectly deleted on subscription's `Unsubscribe()`

Resolves #785
Resolves #776
Resolves #775
Resolves #748
Resolves #747

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
kozlovic added a commit that referenced this issue Aug 15, 2021
They will be described in the release notes, but gist:

Added:
- `DeliverSubject()` option to configure the deliver subject of a JetStream consumer created by the `js.Subscribe()` call (and variants)
- `BindDeliverSubject()` option to subscribe directly to a JetStream consumer deliver subject (bypassing any lookup or JetStream consumer creation)
- Fields `DeliverGroup` in `ConsumerConfig`, `PushBound` in `ConsumerInfo`. They help making prevent incorrect subscriptions to JetStream consumers
- Field `Last` in `SequencePair`

Changed:
- With a `PullSubscription`, calling `NextMsg()` or `NextMsgWithContext()` will now return `ErrTypeSubscription`. You must use the `Fetch()` API
- If the library created internally a JetStream consumer, the consumer will be deleted on `Unsubscribe()` or when the `Drain()` completes
- Fail multiple instances of a subscription on the same durable push consumer (only one active at a time). Also, consumers now have the concept of `DeliverGroup`, which is the queue group name they are created for. Only queue member from the same group can attach to this consumer, and a non queue subscription cannot attach to it. Note that this requires server v2.3.5
- Attempting to create a queue subscription with a consumer configuration that has idle heartbeats and/or flow control will now result in an error

Fixed:
- Possible lock inversion
- JetStream consumers could be incorrectly deleted on subscription's `Unsubscribe()`

Resolves #785
Resolves #776
Resolves #775
Resolves #748
Resolves #747

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
kozlovic added a commit that referenced this issue Aug 15, 2021
They will be described in the release notes, but gist:

Added:
- `DeliverSubject()` option to configure the deliver subject of a JetStream consumer created by the `js.Subscribe()` call (and variants)
- `BindDeliverSubject()` option to subscribe directly to a JetStream consumer deliver subject (bypassing any lookup or JetStream consumer creation)
- Fields `DeliverGroup` in `ConsumerConfig`, `PushBound` in `ConsumerInfo`. They help making prevent incorrect subscriptions to JetStream consumers
- Field `Last` in `SequencePair`

Changed:
- With a `PullSubscription`, calling `NextMsg()` or `NextMsgWithContext()` will now return `ErrTypeSubscription`. You must use the `Fetch()` API
- If the library created internally a JetStream consumer, the consumer will be deleted on `Unsubscribe()` or when the `Drain()` completes
- Fail multiple instances of a subscription on the same durable push consumer (only one active at a time). Also, consumers now have the concept of `DeliverGroup`, which is the queue group name they are created for. Only queue member from the same group can attach to this consumer, and a non queue subscription cannot attach to it. Note that this requires server v2.3.5
- Attempting to create a queue subscription with a consumer configuration that has idle heartbeats and/or flow control will now result in an error

Fixed:
- Possible lock inversion
- JetStream consumers could be incorrectly deleted on subscription's `Unsubscribe()`

Resolves #785
Resolves #776
Resolves #775
Resolves #748
Resolves #747

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed reproducible bug
Projects
None yet
5 participants