-
Notifications
You must be signed in to change notification settings - Fork 142
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
Pubsubstub improvment #496
Comments
@byroot can we gracefully close the open connections? |
Oh right I forgot to mention that. So I'd need to search for the github issues and all again. But AFAIK if you use Sinatra |
Ah so the "solution" would be to use |
Is this finished? |
Well, we do run it externally at Shopify. But it requires quite a bit of custom code to do so. We should make it trivial before we close this issue. |
We're experiencing pain from this as well. @casperisfine, are you able to share the custom code Shopify uses to make this run externally? Even though it is non-trivial, it may help us resolve the problem on our end. Thanks! |
Sure:
stream: "bin/puma --config pubsubstub/puma_config.rb pubsubstub/config.ru --port 8000 --environment $RAILS_ENV"
#!/usr/bin/env puma
# Tells puma to not wait on clients to sutdown
force_shutdown_after 0
# Number of processes
workers 2
# min, max threads per workers
threads 32, 256
require 'yaml'
require 'uri'
require 'rack'
require 'rack/session/redis'
require 'pubsubstub'
require 'bugsnag'
require_relative '../lib/user_required_middleware'
module AppConfig
PATH = File.expand_path('../../config/secrets.json', __FILE__)
extend self
def redis_url
url = URI.parse(config.fetch('redis_url', 'redis://localhost'))
url.port ||= 6379 # EM::Redis is stupid, it need an explicit port
url.path = '/5/session'
url.to_s
end
def redis_session
{redis_server: redis_url, key: "_shipit_session_id_#{RUBY_VERSION}"}
end
def config
@config ||= JSON.load(File.read(PATH).gsub('${HOST_IP}', ENV.fetch('HOST_IP', 'localhost')))
end
def environment
ENV['RAILS_ENV'] || ENV['RACK_ENV'] || 'development'
end
end
class HealthCheck
def initialize(app)
@app = app
end
def call(env)
return @app.call(env) unless env['PATH_INFO'] == '/status/version'
[200, {'Content-Type' => 'text/plain'}, %w(OK)]
end
end
puts "Connecting to #{AppConfig.redis_url}"
Pubsubstub.redis_url = AppConfig.redis_url
Pubsubstub.error_handler = -> (error) { Bugsnag.notify(error) }
use Rack::Session::Redis, AppConfig.redis_session
use HealthCheck
use UserRequiredMiddleware
run Pubsubstub::StreamAction.new Most of the require 'rack'
require 'pubsubstub'
Pubsubstub.redis_url = 'something'
run Pubsubstub::StreamAction.new Except the endpoint can end up public depending what your auth strategy is. Additionally we have a nginx route to forward the SSE traffic to that separate process pool:
Hope this helps. |
@casperisfine thank you! 💯 |
Is this still the recommended way to getting pub sub to work nice? |
Yup! |
@casperisfine It would be really helpful (and safe) if you can provide some rack-idiot-proof guidance on how to implement |
It depends on what authentication method you have enabled. But the one you link should work fine for most people. |
Currently Pubsubstub cause a bunch of minor but annoying issues.
I discussed this a bit with @gmalette, and we think it would be valuable to be able to run it in an external process. That way force reloading it is not an issue.
The default would still be to run it inline though.
The text was updated successfully, but these errors were encountered: