Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #11540 (new defect)

Opened 8 months ago

Last modified 8 months ago

PATCH thread safety for template compilation in ActionView::TemplateHandlers::Compilable

Reported by: coderrr Assigned to: lifofifo
Priority: normal Milestone: 2.x
Component: ActionPack Version: edge
Severity: normal Keywords:
Cc:

Description

I discovered a race condition in ActionView::TemplateHandlers::Compilable#compile_template. Wrapping the method in a mutex fixes the problem.

I have added some comments pointing out the race conditions and included a test (that must be run explicitly) to reproduce them. The test that reproduces them does so by "brute force". I also have written tests which deterministically reproduce the race conditions but they require adding conditional sleep statements to the production code, eg:

sleep Thread.current[:sleep_time_for_race_condition]  if Thread.current[:sleep_time_for_race_condition]

I favored the brute force test as it does not require production code changes.

Attachments

compilable_thread_safety.patch (5.2 kB) - added by coderrr on 04/09/08 20:01:47.
compilable_thread_safety.2.patch (5.2 kB) - added by coderrr on 04/09/08 20:02:34.

Change History

04/07/08 06:44:00 changed by coderrr

  • owner changed from core to lifofifo.

04/07/08 11:21:22 changed by coderrr

accident... the patches are identical

04/09/08 20:01:47 changed by coderrr

  • attachment compilable_thread_safety.patch added.

04/09/08 20:02:34 changed by coderrr

  • attachment compilable_thread_safety.2.patch added.

04/11/08 00:17:07 changed by aok

Should the method always be wrapped in a mutex or only conditionally, when ActionController::Base.allow_concurrency?

04/11/08 00:48:50 changed by coderrr

Very good point.

Ideally it should only be wrapped when allow_concurrency is set. Although, the performance hit for this mutex will be extremely negligible. The mutex is only hit once (during compilation) per template-partials combination. So I'm not sure if it's even worth adding the extra code (and thus complexity) to handle the conditional.

What do you think?