Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Race Condition with rendering parts in goroutines with templ-initialized context #977

Open
kanstantsin-chernik opened this issue Oct 31, 2024 · 3 comments

Comments

@kanstantsin-chernik
Copy link

kanstantsin-chernik commented Oct 31, 2024

In the current implementation, there is an assumption that all the parts are rendered in a single thread. Context value for children is modified for the whole context. If snippets need to be rendered in goroutines, it creates a race condition where children go missing or sporadically added

To Reproduce
https://github.com/kanstantsin-chernik/templ/blob/suspense-async-rendered/examples/suspense-async-rendered/main.templ

I also added comments to the generated code

Expected behavior
Each templ component is independent and can be rendered in a goroutine

Screenshots
Screenshot 2024-11-01 at 1 31 38 PM

Logs

WARNING: DATA RACE
Read at 0x00c01e4a13f0 by goroutine 1053:
  github.com/a-h/templ.GetChildren()
      external/com_github_a_h_templ/runtime.go:78 +0x186

Previous write at 0x00c01e4a13f0 by goroutine 1054:
  github.com/a-h/templ.ClearChildren()
      external/com_github_a_h_templ/runtime.go:68 +0x44

templ info output
% templ info
(✓) os [ goos=linux goarch=amd64 ]
(✓) go [ location=/home/user/go-code/bin/go version=go version go1.23.2 X:nocoverageredesign linux/amd64 ]
(✓) gopls [ location=/opt/go/path/bin/gopls version=golang.org/x/tools/gopls v0.14.2
golang.org/x/tools/[email protected] h1:sIw6vjZiuQ9S7s0auUUkHlWgsCkKZFWDHmrge8LYsnc= ]
(✓) templ [ location=/home/user/go-code/bin/templ version=v0.2.778 ]

Desktop (please complete the following information):

  • OS: all
  • templ CLI version (templ version): v0.2.778
  • Go version (go version): go version go1.23.2 X:nocoverageredesign linux/amd64
  • gopls version (gopls version):golang.org/x/tools/gopls v0.14.2

Additional context

@kanstantsin-chernik
Copy link
Author

A couple of questions:

  • Should Templ support this example by not reusing context or sync rendering is by design?
  • If it is by design, can we at least introduce something like WithoutContext to reset it before we launch go routines?

@kanstantsin-chernik kanstantsin-chernik changed the title Race Condition with rendering parts in goroutine sharing same context Race Condition with rendering parts in goroutine with templ-initialized context Nov 1, 2024
@kanstantsin-chernik kanstantsin-chernik changed the title Race Condition with rendering parts in goroutine with templ-initialized context Race Condition with rendering parts in goroutines with templ-initialized context Nov 4, 2024
@a-h
Copy link
Owner

a-h commented Nov 12, 2024

Hi, just looking into this. Wouldn't you also want to put a mutex around the io.Writer that you're writing to, in order to avoid the danger of multiple components rendering to the writer at the same time?

If you did that, I don't think you would have a race condition. The suspense example in the repo uses a for loop over a channel which means that although data is fetched in parallel, the rendering to the response is sequential, which is typically what you want to prevent multiple writers writing data at the same time.

So, I don't really understand what you're trying to do. Are you thinking that you could get better performance by rendering multiple parts of a page in parallel, or something else?

If it's that you think you'll get better performance by rendering individual sections, then I think you could avoid the race condition by having some HTTP middleware that initializes the context as it receives a request, to enable that, but I suspect that the overhead of creating buffers and copying them around would negate any benefits.

@kanstantsin-chernik
Copy link
Author

kanstantsin-chernik commented Nov 12, 2024

Hi, thanks for taking a look at it!

Lets start with why we are doing this. In our case, we have a lot of heavy snippets rendered in parallel but written sequentially. We saw a 50-60ms saving on paralleling the rendering. Mutex around writer could be added in the async renderer in case of suspence, but for us the order matters, so we write them sequentially. So we do see better performance with rendering in parallel.

If it's that you think you'll get better performance by rendering individual sections, then I think you could avoid the race condition by having some HTTP middleware that initializes the context as it receives a request, to enable that

Unfortunately, it is exactly what doesn't work here. If the context is initialized, than every goroutine would receive the same initialized version of context with the same pointer to Children. Then each of the parallel goroutines starts reading and cleaning Children in a shared pointer, hence the race condition. What we need is to set a tombstone in the context before handing it over to goroutines and make all the goroutines initialize their context again with their own Children pointer

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

No branches or pull requests

2 participants