search

apollographql / graphql-subscriptions

:newspaper: A small module that implements GraphQL subscriptions for Node.js
MIT License
1.59k stars 133 forks source link

Explain the note in README about scaling #120

Open nareshbhatia opened 6 years ago

nareshbhatia commented 6 years ago

The README file says:

It only works if you have a single instance of your server and doesn't scale beyond a couple of connections.

I understand the first part of this sentence - only one server can listen for publish requests - so no horizontal scaling. However what does the second part mean? What limits the implementation to a couple of connections? Do you literally mean two?

vjpr commented 6 years ago

This didn't make sense to me either.

nareshbhatia commented 6 years ago

I have been looking at this for the past few days and the best I can come up with is that graphql-subscriptions uses EventEmitter as its PubSub implementation. EventEmitter, by default, starts throwing up warnings when the number of subscribers exceeds 10 - but that's to avoid inadvertent memory leaks. Here we don't have a memory leak - the number of subscribers is directly proportional to the number of clients we have. So in reality, the implementation may be able to support more than 2 connections.

Please note that I am not questioning that graphql-subscriptions will not scale. It's will at some point - I just don't know when. I am looking for an approach to load test this and other implementations (redis, rabbitmq, ...). Any ideas? Tools like jmeter etc. can do a good job with http request/response, but does anyone have a recommendation on load testing WebSockets and GraphQL subscriptions?

taybin commented 6 years ago

~Maybe the default PubSub implementation could expose setting the MaxListeners() on its EventEmitter(s)?~

I see you can pass in your own EventEmitter to PubSub, negating my suggestion.

nicolaslopezj commented 6 years ago

It’s because when you fire an event on server1, it will send that to it’s connected clients, but that pubsub implementation doesn’t pass events between server, so clients connected to server2 won’t receive the update

grantwwu commented 6 years ago

I think this might just be a general warning against using NodeJS backed things for stuff like this which really use a more performant runtime/that isn't completely in-memory. I don't believe that use of EventEmitter inherently has memory leaks, just that it's not really designed to service large numbers of listeners at once; because it's in Node, it can't run event listeners on multiple threads, which is problematic if work is being done in them. I don't think EventEmitter was designed to replace a system like Redis, Kafka, Cloud PubSub, etc. which are not inherently single-threaded, can be configured to persist to disk, and which can be run in a clustered configuration (what I think @nicolaslopezj) is referring to.

https://github.com/apollographql/graphql-subscriptions/commit/1995496d52a55e0428f1d312c1bfad0b0cb54675 is the commit that added this warning - it contains some more explanation for the warning.

I am open to a clearer/less ambiguous warning.