[FIXED] Possible lock inversion and incorrect delete of JS consumer #792
Conversation
- 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>
If set, the JS consumer will be deleted: - On Unsubscribe(), if error occurs, error is returned. - After Drain (connection and/or subscription) completes. If error occurs there, error is reported through async error cb. Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
| t.Run("durable consumers not deleted on drain", func(t *testing.T) { | ||
| subB, err := js.SubscribeSync("foo.B", nats.Durable("B")) | ||
| t.Run("durable consumers deleted on drain", func(t *testing.T) { | ||
| subB, err := js.Subscribe("foo.B", func(_ *nats.Msg) {}, nats.Durable("B"), nats.DeleteConsumer()) |
There was a problem hiding this comment.
All these changes still needed?
There was a problem hiding this comment.
Meaning? These tests are about checking that consumers are deleted, but the semantic have changed a bit. I could refactor them completely to match the current expectations as opposed to try "fixing" them. Will do that tomorrow.
There was a problem hiding this comment.
Sounds good. Looking at the examples, would you add the opt to Unsubscribe() vs Subscribe()?
There was a problem hiding this comment.
Although it would make more sense, I would not change Unsubscribe()/Drain() (even adding var option may cause backward compatibility issues, and may not be possible in other lang).
|
Closing this PR and will open a new one that is more comprehensive. |
should be done under the subscription lock).
under the protection of the subscription's lock.
called AddConsumer and got a success.
Resolves #775
Resolves #776
Signed-off-by: Ivan Kozlovic ivan@synadia.com