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

sub discard #408

Merged
merged 2 commits into from
Nov 15, 2022
Merged

sub discard #408

merged 2 commits into from
Nov 15, 2022

Conversation

aricart
Copy link
Member

@aricart aricart commented Nov 15, 2022

This is actually an edge case trying a sub that didn't have permissions (on the same subject) due to buffering etc, and because the server doesn't provide the ID the subscription ID that created the problem, writing the test with the same subscription would end up calling the handler for the first subscription twice, and that made a negative test fail.

Now the part that I am not sure about is that if there's a disconnect and in the meanwhile, the permission has changed, the reconnect won't re-establish the sub. Possibly this means dispatching of the error should internally mark that object as reaped while keeping the sub registration open (so reconnect can resub).

This is why this sort of behavior where sub permission errors are not final is kind of a tricky condition. While in some cases R.I. has several where the client will restart multiple times on initialization of an environment, so the sub permission shouldn't be a terminal error, the point is still that the rejected subscription is dead, so even with a server configuration fix, where the permission is re-assigned, that subscription possibly doesn't exist on the server so it is not doing anything until the client restarts.

@aricart aricart changed the base branch from main to dev November 15, 2022 22:26
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

# Conflicts:
#	tests/auth_test.ts
@aricart aricart temporarily deployed to CI November 15, 2022 23:37 Inactive
@aricart aricart merged commit b33a714 into dev Nov 15, 2022
@aricart aricart deleted the sub-discard branch November 17, 2022 13:25
aricart added a commit that referenced this pull request Nov 17, 2022
aricart added a commit that referenced this pull request Nov 17, 2022
…#410)

* [FIX] fixed a condition where a subscription that received a permissions error was not reaped. (#408)

* [FIX] remove remembering of a permissions error (#406)
#406
FIX #530
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