Finding/killing a memory problem in a specific controller in rails

Question

I have a website built on Rails, on Heroku, that typically works fine with about 90% memory usage.

Through Scout I have isolated a problem in my Rails-app where my comments#create-controller sometimes allocates 860k of Memory which shuts down my app for a long time in the time-outs etc that follow. Most of the time the allocated memory is a fraction of it so the problem is intermittent.

The comment-function itself is not super important but I still need it. I believe three different parts of it could cause this memory problem:

1. The content string (i.e. the comment itself) is too long. For example if a spammer posts a super long text. I don't believe this to be the issue as my last memory spike was caused by a normal user, posting a very short comment.

2. My rakismet-gem (https://github.com/joshfrench/rakismet) and spam check. I am using the latest version (1.5.4). It could be likely this is a problem as I don't really know what is loaded into the memory when it is being used.

3. My Notifier-call in the code.

   • Is there anything I can do to catch memory problems and rescue in the controller so if there are any "bad" comments, they wont break the entire site?

   • Do you see anything that could cause this monster memory allocation in the code?

Code below:

Comments#Create:

  def create    
    require 'memory_profiler'
    report = MemoryProfiler.report do

    @comment = Comment.new(comment_params)
    spam_features = %w(\xA cialis informative the that this buy href)
    unless @current_administrator.present?
      if spam_features.any? {|str| @comment.content.include? str}
        logger.info "L: Comment include spam features"
        redirect_to article_path(Article.find('din-kommentar-har-inte-sparats')) and return 
      elsif @comment.author.size > 40 || @comment.author_email.size > 40
        logger.info "L: Comment author name or email too long (suspicious)"        
        redirect_to article_path(Article.find('din-kommentar-har-inte-sparats')) and return       
      end
    end

    # This shouldn't be here (but don't know how to put it in the model)
    if !@comment.blog_post_id.blank? # This is a comment on a blog post
      return_to_path = blog_post_path(BlogPost.find(@comment.blog_post_id))
    elsif !@comment.gift_id.blank? # This is a comment on a gift
      return_to_path = gift_path(Gift.find(@comment.gift_id))      
    elsif !@comment.contest_id.blank? # This is a comment on a contest     
      return_to_path = contest_path(Contest.find(@comment.contest_id))   
    elsif !@comment.christmas_fair_id.blank? # This is a comment on a christmas fair     
      return_to_path = christmas_fair_path(ChristmasFair.find(@comment.christmas_fair_id))
    elsif @comment.tmp_julrim # This is a comment on a christmas fair     
      return_to_path = rhymes_path                   
    else
      raise ActionController::RoutingError.new('Not Found')
    end
    return_to_path << "#comments"
    @comment.status_id = 3

    @comment.user_ip = request.remote_ip
    @comment.user_agent = request.env['HTTP_USER_AGENT']
    @comment.marked_as_spam = @comment.spam? # Using rakismet to check for spam
    #if !@comment.marked_as_spam || @current_administrator.present?
    respond_to do |format|      
      #@comment.status_id = 1 if @comment.contest_id == 44          
      if @comment.save
        Notifier.new_comment(@comment).deliver if Rails.env == 'production' unless @comment.marked_as_spam
        format.html { redirect_to return_to_path, notice: 'Din kommentar har registrerats och kommer att ses över innan den godkänns.' }
        # format.json { render action: 'show', status: :created, location: @comment }
      else
        format.html { render action: 'new' }
        format.json { render json: @comment.errors, status: :unprocessable_entity }
      end      
    end

    end

Answer 1

one thing that stands out to me is your turfing else statement

raise ActionController::RoutingError.new('Not Found')

which has a raise. Just render a 401 here. You already know it's a 401 which avoids the raise through the stack. Also this whole logic could be moved to a dedicated protected method. Here is how I would refactor your method with comments.

# always do requires in the file before the class definition
# so this would go at the top of the file
require 'memory_profiler'

...

def create    
  report = MemoryProfiler.report do
    @comment = Comment.new(comment_params)
    check_admin?  

    # There is possibility to merge these with the comment params above 
    # during init above or just pass them to the model and act upon 
    # appropriately  there
    @comment.status_id = 3
    @comment.user_ip = request.remote_ip
    @comment.user_agent = request.env['HTTP_USER_AGENT']
    @comment.marked_as_spam = @comment.spam? # Using rakismet to check for spam

    #if !@comment.marked_as_spam || @current_administrator.present?
    respond_to do |format|      
      if @comment.save
        Notifier.new_comment(@comment).deliver if Rails.env.production? && !@comment.marked_as_spam
        format.html   { 
          if return_to_path == false
            render file: "public/401.html", status: :not_found # dump to 401 immediately
          else
            redirect_to return_to_path, notice: 'Din kommentar har registrerats och kommer att ses över innan den godkänns.' 
          end
        }
        # format.json { render action: 'show', status: :created, location: @comment }
      else
        format.html { render action: 'new' }
        format.json { render json: @comment.errors, status: :unprocessable_entity }
      end      
    end
  end
end

protected 

  def spam_features
    %w(\xA cialis informative the that this buy href)
  end

  def return_to_path 
    anchor = "comments" 
    if @comment.blog_post_id.present?
      blog_post_path(@comment.blog_post, anchor: anchor) # trust your associations vs. relookup and leverage the anchor option in url helpers
    elsif @comment.gift_id.present?
      gift_path(@comment.gift, anchor: anchor) # trust your associations vs. relookup and leverage the anchor option in url helpers
    elsif @comment.contest_id.present?
      contest_path(@comment.contest, anchor: anchor) # trust your associations vs. relookup and leverage the anchor option in url helpers
    elsif @comment.christmas_fair_id.present?
      christmas_fair_path(@comment.christmas_fair, anchor: anchor) # trust your associations vs. relookup and leverage the anchor option in url helpers
    elsif @comment.tmp_julrim
      rhymes_path(anchor: "comments") and leverage the anchor option in url helpers                   
    else
      false # give a testable exit condition and for short circut render
    end 
  end

  # if you were to check the comment_params vs an instantiated object, you could 
  # short circuit the controller method in a before_action 
  # Also check out known existing methods of spam prevention such as invisible_captcha or rack attack. Ideally 
  # once you hit your controller's method spam checking is done. 
  def check_admin? 
    # for clarity use positive logic check when possible, e.g. if blank? vs unless present? 
    # reduce your guard code to one the fewest levels necessary and break out into testable methods
    if has_spam? 
      logger.info {"L: Comment include spam features"} # use blocks for lazy evaluation of logger
      redirect_to article_path(Article.find('din-kommentar-har-inte-sparats')) and return 
    elsif has_suspicious_name? 
      logger.info {"L: Comment author name or email too long (suspicious)"} # use blocks for lazy evaluation of logger
      redirect_to article_path(Article.find('din-kommentar-har-inte-sparats')) and return       
    end
    # is there be an else condition here that we're not accounting for here? 
  end

  # this check is less than optimal, e.g. use of any? and include? has code smell
  def has_spam? 
    @current_administrator.blank? && spam_features.any? {|str| @comment.content.include? str } 
  end

  def has_suspicious_name?
    @current_administrator.blank? && @comment.author.size > 40 || @comment.author_email.size > 40
  end

Answer 2

The standout issue is this:

Notifier.new_comment(@comment).deliver if Rails.env == 'production' unless @comment.marked_as_spam

I'm assuming this is an ActionMailer object. deliver is a blocking method and not something you'd usually want to use in production during the request-response cycle. This could cause major delays if your mail server is slow to respond, so you should replace it with deliver_later and ensure you have a tool like Sidekiq available to fulfill the request in the background.

(deliver is deprecated as of Rails 5 btw, in favour of deliver_now and deliver_later.)