search

aserafin / grape_logging

Request logging for Grape!
MIT License
147 stars 76 forks source link

Fix serious memory leak #7

Closed marshall-lee closed 9 years ago

marshall-lee commented 9 years ago

Middleware objects are temporary but ActiveSupport::Notifications subscription is global.

Closure (created by middleware instance during single request) survives further garbage collections. Closure object reaches middleware instance, middleware instance reaches env hash and response object (containing status code and body btw!) so they are all cannot be collected. There is a lot of leaking per-request memory that stays alive after the request is finished!

dmitry commented 9 years ago

@aserafin hopefully it will be merged very soon.

aserafin commented 9 years ago

Thanks, I've totally overlooked it :)

aserafin commented 9 years ago

Pushed to rubygems https://rubygems.org/gems/grape_logging/versions/1.1.1

marshall-lee commented 9 years ago

btw this flaw is very serious. it's similar to saving every response body to memory. it was bloating our servers memory. application server process could reach about 3 GB memory consumption or more.

aserafin commented 9 years ago

yes, I understand that. that's why it was merged and new version was released so quick

marshall-lee commented 9 years ago

thanks!

marshall-lee commented 9 years ago

Besides memory leakage this bug also slows down CPU.

Suppose our application process served 1_000_000 requests before. So one million callbacks will be registered. Each executing sql query will trigger million callbacks!

dmitry commented 9 years ago

@marshall-lee is this PR have fixed all your leakage issues?

marshall-lee commented 9 years ago

@dmitry We are testing it right now in a working application.

But in a sample application ObjectSpace tracing is not showing leakage of middleware instances anymore. Before this patch every request leaked constantly three Grape::Middleware instances.

marshall-lee commented 9 years ago

I also don't know what will be in the case of unhandled exception. Does it still go through this middleware?

dmitry commented 9 years ago

@marshall-lee looks like it is: https://github.com/aserafin/grape_logging/blob/master/lib/grape_logging/formatters/default.rb#L12

aserafin commented 9 years ago

@dmitry @marshall-lee but that's only if you explicitly want to do it in the rescue :all block, see here https://github.com/aserafin/grape_logging#logging-exceptions

marshall-lee commented 9 years ago

@dmitry @aserafin please see #8