Skip to content

Make Redcarpet::Markdown#render thread safe - #672

Open
tessereth wants to merge 2 commits into
vmg:masterfrom
tessereth:thread-safe
Open

Make Redcarpet::Markdown#render thread safe#672
tessereth wants to merge 2 commits into
vmg:masterfrom
tessereth:thread-safe

Conversation

@tessereth

Copy link
Copy Markdown

I ran into the same issue as #570 and I've been trying to think of a good way to fix the issue. The solution is not entirely obvious but as far as I can tell, there are a few options:

  1. Update the documentation to say that Redcarpet::Markdown is not thread safe and you should make one per thread.
  2. Add a global rendering lock. That's what I have here. Based on benchmarks on my laptop, this has an ~2% performance hit for the single threaded case and you get no benefit from concurrency but it doesn't just crash the program.
  3. Split the state into the fixed state (that can be shared between threads) and the rendering state (that needs to be separate). This was the suggestion in that issue. I made a rough attempt at that but sd_markdown->work_bufs is used to cache memory buffers between renders. This doesn't work in the concurrent world and if we just re-create them for every render there's a noticeable performance hit on subsequent renders using the same Redcarpet::Markdown object (~7%). Plus the active encoding is stored in the renderer options making that also not thread safe so we'd have to split those options in two as well.

While I could probably implement option 3, I'm not sure it's possible without at least a minor performance hit. So my questions for you are

  1. Is this a thing you want fixed?
  2. Is this a thing you want fixed enough to warrant a performance hit? If so how much?
  3. Do you think there's value in actually making render concurrent? It's fairly likely this will speed up the concurrent case but slow down the sequential case.

ClearlyClaire added a commit to ClearlyClaire/mastodon that referenced this pull request May 23, 2019
ClearlyClaire added a commit to glitch-soc/mastodon that referenced this pull request May 23, 2019
rtucker pushed a commit to vulpineclub/mastodon that referenced this pull request Jan 7, 2021
@MatheusRich

Copy link
Copy Markdown

@boazsegev @robin850 we've recently ran into a nasty bug related to this. We were saving a rendered in a constant and that caused weird errors. I'd love to get this merged, so other people won't fall into the same problems.

Is there anything missing here or needed to move it forward?

@boazsegev

Copy link
Copy Markdown

I think there are two main use cases:

  1. rendering templates before pushing for production - performance hit doesn't matter
  2. rendering on the fly - risk of data loss is unacceptable, performance hit is okay

My 2¢.

@david-a-wheeler

Copy link
Copy Markdown

At the least, it should be clearly documented as not thread-safe. I plan to make several objects and attach them to Thread.current. I just needed to know that I needed to do it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants