sighmon / mjml-rails

MJML + ERb/Haml/Slim view template
https://mjml.io
Other
300 stars 65 forks source link

Fix version check to clean up after itself ๐Ÿ‘ฎโ€โ™€๏ธ #29

Closed klappradla closed 6 years ago

klappradla commented 6 years ago

Hi ๐Ÿ‘‹

first of all: thanx a ton for building an maintaining this gem ๐Ÿ’š It's really helpful ๐Ÿค—

This PR fixes the check for the mjml version to clean up after itself and not leave a node zombie process lingering around ๐ŸงŸโ€โ™‚๏ธ

scenario

We're using Sidekiq to send emails in background jobs and our workers always had some "defunct" node processes lingering around forever - even directly after starting / restarting Sidekiq.

problem

This line in mjml.rb ๐Ÿ‘‡

IO.popen("#{bin} --version").read.include?('mjml-core: 4.0.')

This runs mjml --version as a subprocess. But since the IO object is never closed, the process never gets terminated.

solution

Looking at the docs for IO#popen, the method can also take a block, like so:

IO.popen("#{bin} --version") do |io|
  io.read.include?('mjml-core: 4.0.')
end

โ˜๏ธ this will do the exact same as the example above, but also clean up after itself, closing the pipe again. The docs on that:

If a block is given, Ruby will run the command as a child connected to Ruby with a pipe. Ruby's end of the pipe will be passed as a parameter to the block. At the end of block, Ruby closes the pipe and sets $?. In this case IO.popen returns the value of the block.

So I think this is the better fit for the version check: no zombie node processes lingering around any more ๐ŸŽ‰

sighmon commented 6 years ago

@klappradla You are the actual best. Thanks so much for your fix, and the cutest pull request I've ever had. ๐Ÿ™Œ๐Ÿผ

klappradla commented 6 years ago

Thanx for your super fast response @sighmon ๐Ÿ™

It would be awesome if you could release a patch version soon, so that our operations people have a calm sleep ๐Ÿ›Œ and I'm no longer pulling in dependencies straight from Github โœŒ๏ธ

sighmon commented 6 years ago

@klappradla 4.0.2 should be available in Rubygems now. :-)

klappradla commented 6 years ago

๐Ÿค— thanx a ton!