-
-
Notifications
You must be signed in to change notification settings - Fork 387
fix: rate limit presence events #1493
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
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
786f7ef
to
e23258e
Compare
73957f4
to
fb7cda7
Compare
f1d7589
to
a0ece8d
Compare
@@ -706,7 +706,6 @@ defmodule Realtime.Integration.RtChannelTest do | |||
|
|||
WebsocketClient.send_event(socket, topic, "presence", payload) | |||
|
|||
assert_receive %Phoenix.Socket.Message{topic: ^topic, event: "phx_reply", payload: %{"status" => "ok"}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why this has changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be here as this could be an error actually as this event would mean it reconnected 👀
4f1ce27
to
49402ce
Compare
49402ce
to
f923e1d
Compare
) do | ||
%{assigns: %{authorization_context: authorization_context}} = socket | ||
defp handle_presence_event("track", payload, db_conn, socket) | ||
when can_write_presence?(socket) and is_nil(socket.assigns.policies.presence.write) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Maybe here it could be only when is_nil(socket.assigns.policies.presence.write)
which means we still need to calculate just for this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did this change as it also simplifies a lot the guard of the function
push(socket, "presence_state", presence_dirty_list(topic)) | ||
{:noreply, socket} | ||
@spec sync(Socket.t()) :: | ||
:ok | {:error, :rls_policy_error | :unable_to_set_policies | :rate_limit_exceeded} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this function return rls_policy_error or unable_to_set_policies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no! copy pasta strikes again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments but overall LGTM. Nice one
🎉 This PR is included in version 2.42.6 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
What kind of change does this PR introduce?
rate limit presence events