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

Send commands based on transport state instead of connected state #278

Conversation

ThisIsEsh
Copy link
Contributor

@ThisIsEsh ThisIsEsh commented Apr 4, 2024

Change sending commands logic, rely on transport state instead of client connected state,

Fixes missing subscribe frames bug for unsubscribes, that have been called between transport open and connected frame response for server.

Also it will allow to send subscribe requests after transport has been opened without waiting for response from server.

Notes:

Removed optimistic flag from Subscription, it does not make sense, because we can just rely on transport state.

Removed skipSending flag from Subscription, it's not longer required, because it have been used only for sending optimistic subs in emulation transport and this optimistic subs are removed right now, because they would require implementation of queue of unsubscribe frames, which would lead to way more complicated logic. But it will be easy to return, if will be required later.

Also I have made a public method in centrifuge, that indicates that transport is ready, so it can be used inside subscription without @ts-ignore, but I think it would be good idea to move all logic of skipping commands sending into Centrifuge client and remove early returns from Subscription.

P.S. I have spent some time trying to properly configure centrifugo server instance to be able to run all tests locally, what do you think about adding docker-compose file + config.json to this repo?

@FZambia
Copy link
Member

FZambia commented Apr 5, 2024

Many thanks @ThisIsEsh

Will take a look today and during the weekend.

Regarding local tests, there is a single command used in CI:

run: docker run -d -p 8000:8000 -e CENTRIFUGO_PRESENCE=true centrifugo/centrifugo:latest centrifugo --client_insecure --http_stream --sse

But adding docker-compose makes sense anyway - let's solve separately - #279

Copy link
Member

@FZambia FZambia left a comment

Choose a reason for hiding this comment

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

This looks great, efficient and simplifies implementation a lot. Found a couple of things to fix. Could you please check it out, and I'll make one more iteration after that.

src/centrifuge.ts Outdated Show resolved Hide resolved
src/centrifuge.ts Outdated Show resolved Hide resolved
src/subscription.ts Outdated Show resolved Hide resolved
@FZambia FZambia changed the title Change sending commands logic Send commands based on transport state instead of connected state Apr 5, 2024
@FZambia FZambia self-requested a review April 7, 2024 03:59
Copy link
Member

@FZambia FZambia left a comment

Choose a reason for hiding this comment

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

This is 💎, many thanks @ThisIsEsh

@FZambia FZambia merged commit d561a6a into centrifugal:master Apr 7, 2024
2 checks passed
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.

2 participants