Bring Smashing into 2017 #64
Conversation
- Keep track of events on per-client basis - Clients get a UUID - If a client's event queue grows too big it gets dropped - If a Puma thread tries to send the next batch of events to a client and it's event queue is dropped, the queue will be created - Updated tests to match above changes
- Realized that Rufus can also be multi-threaded now - Added a mutex for each connection, allowing safe access during send_event - Don't need to track a queue of events per connection - should lower memory footprint - Connection termination detected by catching Puma::ConnectionError exceptions during send_event conntion writing - Updated tests accordingly
- configure puma using config/puma.rb config file - add default puma.rb config file for new projects - adapt dashing cli to use the config file and correctly handle -d (daemonize) arg. - adapt cli_test.rb to the new setup - remove tmp dir from gitignore as we want to generate it in the project template - add tmp/pids/ dir in project template (for saving puma pid / state files.
There's literally no reason to not just start/stop puma directly.
|
This sounds like great enhancements, thanks a lot. Won't have time to review this next two weeks as I'm taking a new role at work. But one thing that concerns me is that we just released 1.0.0, and this addresses several good points, but comes with no backward compatibility, and the pull request is quite big. The issue is that users would have to adapt start scripts, dependencies, verify that their jobs are all working; and in case we have to revert, we might have to spend some time looking through the commits to guess what happened where. @clemens do you think some of these changes could be postponed? Perhaps we could release a new gem with all the changes that are backward compatible, deprecating whatever is going to be released in the upcoming release, and give users some time to review it? |
|
Hey, thanks for your work on this. The Puma change is not simple, trust me -- I made those commits. They're ugly as sin. Sinatra doesn't implement rack hijack correctly. Until it does, this change is basically impossible without doing a bunch of nasty hackery / re-implementing the webserver. More details on that here: Shopify/dashing#677. Here was my list of things to bring it up to 2016: #8 (comment). IMHO the biggest thing that needs fixing is Batman.js. It should be replaced with Vue.js or React. Also, removing start/stop from the service is not acceptable in any sense. |
|
Not sure I can follow your line of argumentation apart from the Puma change (I wasn't aware of these things and had no issues when I tested this locally). Yes, Batman.js should be replaced by Vue/React. I'd by the way also argue that server-sent events should be replaced with Websockets since they're supported by all major platforms (including IE/Edge). But does that mean development here is essentially frozen until somebody finds the time for a complete rewrite? Because in my mind that doesn't make sense: If there's no time/muse for a full rewrite, shouldn't tools just gradually improve if people contribute? This especially holds true considering that keeping things as they are would force people to keep using outdated and potentially buggy (functional issues as well as security holes) versions of gems that have developed well over the years. Also, could you explain why removing start/stop from the CLI "is not acceptable in any sense" because I really don't understand? What's the value of having a wrapper script that adds exactly nothing in terms of functionality? Usually, commands like these are used as wrappers around different adapters – e.g. if the tool would support both Thin and Puma and they had different parameter names for the same options, there would be a benefit to, say, use I'm not trying to be arrogant here and I'm not saying that my changes are perfect in any way – I simply don't understand the points your trying to make. Happy to hear your thoughts. |
|
Having a fixed interface to stop/start is a big value on its own. |
|
On the smashing start & stop, I would have to update a few internal docs (and hopefully won't have to do that again if we ever switch from puma to something else?). But the main issue would have to update start up scripts in Linux boxes. Some are maintained manually, others by Ansible/SaltStack. I would have to check if the start-up script is firing Then again, if we ever change the puma command, we would have to update again I think? Your points on security are good, and I agree that we must fix these. I have one instance running Smashing with the latest Gridster, but haven't looked at other libraries yet. Separate issues / pull requests will be much easier to fix and review though. |
Alright, that I can understand.
So basically you'd want a separate PR for the gem updates and the Puma change should be shelved/removed? |
This pull request fixes a few things that are, in my opinion, overdue.
smashing startandsmashing stopcommands: There are no benefits and it unnecessarily bloats the CLI API. Simply use Puma's default commands insteadI hope this all makes sense.