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
Add SSE functionality for rails based on AC::Live #10040
Conversation
|
|
| SSE_FIELDS = OPTIONAL_SSE_FIELDS + REQUIRED_SSE_FIELDS | ||
|
|
||
| module ClassMethods | ||
| @@sse_clients = {} |
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 doesn't seem to be thread-safe.
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.
I think this might actually be fine since @@sse_clients is Hash that is only ever written into during an app boot, am I right @dahakawang? However why use a global hash at all? Why not use a plain ivar, by changing self.extended (and other methods) like this:
def self.extended(base)
base.class_exec do
# the entry point for a Controller level SSE source
def sse_source
start_serve self.class.client_list
end
@sse_clients = []
end
endThere 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.
@thedarkone yes, i think this is a better way to do something like 'mixin a class var' .
ill update my code to use your solution, thx
|
On a closer look I see that it keeps a collection of clients on a controller instance. Taking aside thread-safety concerns, I see other major drawbacks of this approach. This will be problematic in clustered environments (any multi-process setup). For example, Puma with a few workers, or a few EC instances that run one app, or even Heroku. The problem is that writing in one worker will not have any effect on subscribers in other workers. While I think Client level SSE part may prove useful addition, Controlller level SSE is inherently flawed and will cause more confusion than solve real problems. |
include ActionController::ServerSentEvents
extend ActionController::ServerSentEvents::ClassMethodsIs there a motivation for not using |
|
Since it's been over 2 years since this pull request was last committed to, is it safe to assume it should be closed? Is anyone interested in continuing it? |
|
r? @tenderlove Thoughts on this feature? |
|
I like the feature. Is there a way we can implement this as a gem first though? |
This class is an initial implementation of HTML5 SSE for rails. Currently we provide 2 kinds of ways to implement a SSE server. The first is controller level SSE, in which we do not distinguish clients and will send data to all the clients that subscribe the same sse source.
The second is client level SSE, which we DO distinguish clients and the data sent to different client can be totally different.
Controlller level SSE:
in this way, an action named
sse_sourcewill be created automaticlly and you can useMySSE.send_sseorMySSE.send_sse_hashto send event to all clients that subscribed the sse_source (you should register the action in route.rb manually)Client level SSE(Session awared):
Please note that Controller level SSE and Client level SSE are not meant to
work together in the same controller.