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

request-response cycle does NxN work when it could do Nx1 #407

Closed
doodlesbykumbi opened this issue May 17, 2021 · 7 comments
Closed

request-response cycle does NxN work when it could do Nx1 #407

doodlesbykumbi opened this issue May 17, 2021 · 7 comments
Milestone

Comments

@doodlesbykumbi
Copy link

doodlesbykumbi commented May 17, 2021

I noticed that when a request for all sockets is issued every client gets the request (expected), and every client gets the response (hmmm?). The clients getting every response means we're consuming NxN resources for something that could be done with N.

this.pubClient.publish(this.responseChannel, response);

This seems to be because the response channel is singular^. A path forward could be to have the request originator start listening on a channel associated with the request id then have the responses come through this temporary channel.

@doodlesbykumbi doodlesbykumbi changed the title request-response cycle spawns NxN packets request-response cycle does NxN work when it could do Nx1 May 17, 2021
@doodlesbykumbi
Copy link
Author

doodlesbykumbi commented May 18, 2021

I've added a draft PR^ showing how the approach would work. It could certainly be made cleaner, especially around the finaliser stuff. It could be made backwards compatible by counting the number of subs on the response channel, if that value is non-zero then fall back to the old behaviour.

@doodlesbykumbi
Copy link
Author

Ping @darrachequesne

darrachequesne added a commit that referenced this issue Nov 29, 2021
Previously, a request would be sent to all listening nodes, on channel
`${key}-request#${nsp}#` (e.g. "socket.io-request#/#"), and the other
nodes would respond on a common channel `${key}-response#${nsp}#`, so
every node get the response, instead of only the requesting node.

This commit adds a new option: "publishOnSpecificResponseChannel". If
it's set to true, then the other nodes will now respond on
`${key}-response#${nsp}#${uid}#`, which is the channel specific to the
requesting node, thus reducing the noise for the other nodes.

To upgrade an existing deployment, users will need to upgrade all nodes
to the latest version with publishOnSpecificResponseChannel = false,
and then toggle the option on each node.

Note: the option will default to true in the next major release

Related: #407
@darrachequesne
Copy link
Member

Implemented in f66de11.

@darrachequesne darrachequesne added this to the 7.1.0 milestone Nov 30, 2021
@dr-js
Copy link

dr-js commented Mar 26, 2024

@darrachequesne should we change the default publishOnSpecificResponseChannel to true ?

Currently this config is still false in v8.3.0:
https://github.com/socketio/socket.io-redis-adapter/blob/8.3.0/lib/index.ts#L132-L133

@darrachequesne
Copy link
Member

@dr-js yes, we totally could, though that would be a breaking change.

Since this would trigger a new major release, we could even make it the default and remove the publishOnSpecificResponseChannel option.

@dr-js
Copy link

dr-js commented Mar 27, 2024

I would suggest a new major release, as this should reduce response count massively when using fetchSockets().

And in release changelog we could tell existing user to enable the option and gain possible performance.

@dr-js
Copy link

dr-js commented Apr 9, 2024

@darrachequesne after some cluster mode testing with redis-cli monitor,
I found namespace.serverSideEmit() should also use publishOnSpecificResponseChannel to reduce response,
currently callback for serverSideEmit still broadcast to non-specific channel,
related pull request: #536

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants