-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
Scalability of beacons, max-timeouts, and distributed systems #45
Comments
This is a bug, I'll fix it soon.
I'll be honest, I don't think something like I do agree with this though:
I think a new
Sure, will do.
No need for more dependencies, we already know when the server side stream stops We just need to delete the map entry. Main issue
This is an architecture issue. Take a look at this https://learn.microsoft.com/en-us/azure/application-gateway/configuration-http-settings#cookie-based-affinity I really don't understand why Cloudflare doesn't allow for such configuration, it's a no brainer. I did bump into a similar issue in the past, although we were having a slightly different use case, not SSE. You can even hop between sessions if you save your affinity cookie and swap the cookie by using your own "gateway" let's say. There's not much I can do to help you with this part, other than: try other providers. That being saidI might have found a way to deal away completely with beacons. It doesn't fix your main issue regarding nodes, but it will simplify the api a lot. PSNo need to split this into multiple issues. |
I just released patch 0.12.13, which addresses the beacon variance issue and the memory leak. Breaking changes will come next in order to drop beacons. |
Hello @oscarhermoso , I just released version 0.13.0. As mentioned before - beacons are gone. You should check the docs, the api has changed a bit. The new mechanism through which we detect if a client is still connected or not doesn't rely on the client itself at all anymore. The detection is done by the server alone now. Other than the data of your nodes, which you should centralize somehow (maybe through a database) everything should be consistent in a serverless environment now. I think this should fix your main issue, can you confirm? |
I have skimmed through the code, this looks like a really good change. I am a little too familiar with Azure's session affinity on their App Service / App Gateway / Front Door resources 😅 It is useful/necessary for a lot of legacy apps, but in the long run I have found that it hurts more than it helps. It reduces the effectiveness of load balancing, slows down connection draining, and sometimes developers over-rely on it, resulting in broken user-experiences when backend nodes are swapped out or restarted... It is a massive improvement to remove beacons and have I will be able to test the implementation next weekend, will leave the ticket open until then. |
I'm closing this issue due to inactivity. |
Thanks @razshare, much appreciated again |
Issues
There are a few issues with the current
sveltekit-sse
beacon/timeout implementation that make it difficult to use in large production systems:Must fix
timeouts
andlocks
inevents.js
will only grow in memory, as expired keys are never removedextend
the timeout.Nice to have
Below issues can be solved in user-land, so definitely not critical, but raising for discussion anyway:
maxTimeout
or similar due to proxies like Cloudflare expecting responses to complete within 100 seconds.beacon
andmaxTimeout
parameters to prevent thundering-herd scenarios?timeout
andbeacon
values. I have not been able to find a definitive guide on what is "best practice", but something likebeacon: 30
,timeout: 45
seems to be appropriate?Fix
Problems 1 and 2 can be solved by following the approach of sveltekit-rate-limiter and use @isaacs/ttlcache instead of Map, with an option to swap out the cache backend.
Alternatively, could use setTimeout etc. to clear expired map entries but the option to swap out the cache backend would still be necessary for distributed systems.
If you are happy for the library to support a
maxTimeout
parameter, it would be good if it used the same cache implementation too.4/5 can be split into different GitHub issues for discussion but felt they were related enough to be raised in the same ticket.
Workaround
I can work around most of these problems for now by simply setting
beacon: 0
,timeout: 60 + Math.random() * 30
, alongside a nightly restart on my server to reset the memory usage.The text was updated successfully, but these errors were encountered: