Possible race condition in the Maildown::MarkdownEngine.default_html_block method

[yuki24]

I looked into what has caused the bug ruby/did_you_mean#102. One thing I noticed is that in the maildown gem, the kramdown gem is lazily loaded on the fly, which adds a handful of constants dynamically. In an extreme condition, this may cause a race condition. Consider the following scenario where a Sidekiq instance runs two threads (workders) that attempt to run an email delivery job:

Thread 1	Thread 2
calls MarkdownEngine.default_html_block	-
-	calls MarkdownEngine.default_html_block
attempts to require 'kramdown' for the first time	-
evaluates the line that adds a Kramdown constant but doesn't reach the line 74 that adds Kramdown::Document	-
-	evaluates unless defined? Kramdown (now evaluates to unless true)
-	attempts to call html_block.call on the proc that has a reference to the Kramdown::Document that does not exist yet

In this scenario, the thread 2 would raise a NameError because the Kramdown constant is loaded but the Kramdown::Document is not. Also, it would only occur once after a cold reboot since the constant would be available once it's loaded by the thread 1.

I wasn't sure what fixes you'd prefer, so just wanted to bring this up. Thanks!

[schneems]

Thanks for the note. The reason that it's a lazy load is that people can install their own markdown rendering engines if they want (though honestly I have no proof that anyone is doing that.

I think the fix would be requiring the gem at boot time, but rescuing in-case it does not exist. The only other thing I could think of is to use a mutex to block multiple clients from trying to require at the same time, but that increases a tiny bit of runtime overhead that I don't think is needed.

[schneems]

Do you know if Autoload fixes this issue, or if I need to mutex it?

[yuki24]

I was late by a hair, but you may have to mutex it. There is a thread-safety issue in Autoload https://bugs.ruby-lang.org/issues/12688 and the example in the issue happens to reproduce the exact same issue you saw a few days ago. Not sure that was fixed.

cc @ko1

[schneems]

Looks like it is threadsafe https://twitter.com/schneems/status/978705647301746688

[bf4]

It's supposed to have been thread safe since 2.0 headius/thread_safe#15 (comment)