fix(ifchanged): use per-execution state instead of shared AST node state

Author: flosch

pongo2_issues_test.go

@@ -78,3 +78,74 @@ func TestBugCycleSharedState(t *testing.T) {
 	}
 	wg.Wait()
 }
+
+func TestBugIfchangedSharedState(t *testing.T) {
+	// Bug: tagIfchangedNode.lastValues and lastContent were stored on the
+	// AST node, which is shared across all concurrent executions.
+	// This caused two problems:
+	// 1. Data race: concurrent writes to lastValues/lastContent
+	// 2. Semantic bug: ifchanged compares against state from a DIFFERENT
+	//    execution, so the first item might be suppressed if a previous
+	//    execution ended with the same value.
+	//
+	// Each template execution must have independent ifchanged state.
+
+	tpl, err := pongo2.FromString(`{% for item in items %}{% ifchanged %}{{ item }}{% endifchanged %}{% endfor %}`)
+	if err != nil {
+		t.Fatalf("failed to parse template: %v", err)
+	}
+
+	ctx := pongo2.Context{"items": []string{"a", "a", "b", "b", "c"}}
+	const expected = "abc"
+
+	// First: verify sequential executions produce consistent results.
+	// With shared state, the second execution starts with lastContent="c"
+	// from the first execution, so "a" would be correctly shown (different
+	// from "c"), but the pattern breaks in more complex scenarios.
+	// Use a case where it definitely breaks: items starting with the same
+	// value the previous execution ended with.
+	tplSameEnd, err := pongo2.FromString(`{% for item in items %}{% ifchanged %}{{ item }}{% endifchanged %}{% endfor %}`)
+	if err != nil {
+		t.Fatalf("failed to parse template: %v", err)
+	}
+
+	// Items end with "x", so next execution starting with "x" would skip it
+	ctxEndsX := pongo2.Context{"items": []string{"a", "b", "x"}}
+	ctxStartsX := pongo2.Context{"items": []string{"x", "y", "z"}}
+
+	// First execution ends with lastContent="x"
+	result1, err := tplSameEnd.Execute(ctxEndsX)
+	if err != nil {
+		t.Fatalf("execution 1: unexpected error: %v", err)
+	}
+	if result1 != "abx" {
+		t.Errorf("execution 1: got %q, want %q", result1, "abx")
+	}
+
+	// Second execution should output "x" even though previous ended with "x"
+	result2, err := tplSameEnd.Execute(ctxStartsX)
+	if err != nil {
+		t.Fatalf("execution 2: unexpected error: %v", err)
+	}
+	if result2 != "xyz" {
+		t.Errorf("execution 2: got %q, want %q (ifchanged state leaked from previous execution)", result2, "xyz")
+	}
+
+	// Also verify concurrent executions produce correct results.
+	var wg2 sync.WaitGroup
+	for i := 0; i < 20; i++ {
+		wg2.Add(1)
+		go func() {
+			defer wg2.Done()
+			result, err := tpl.Execute(ctx)
+			if err != nil {
+				t.Errorf("concurrent: unexpected error: %v", err)
+				return
+			}
+			if result != expected {
+				t.Errorf("concurrent: got %q, want %q", result, expected)
+			}
+		}()
+	}
+	wg2.Wait()
+}

tags_ifchanged.go

@@ -4,15 +4,33 @@ import (
 	"bytes"
 )
 
-type tagIfchangedNode struct {
-	watchedExpr []IEvaluator
+// ifchangedState holds the per-execution mutable state for an {% ifchanged %} tag.
+type ifchangedState struct {
 	lastValues  []*Value
 	lastContent []byte
+}
+
+type tagIfchangedNode struct {
+	watchedExpr []IEvaluator
 	thenWrapper *NodeWrapper
 	elseWrapper *NodeWrapper
 }
 
+// getState returns the per-execution ifchanged state for this node,
+// creating it on first access. Each template execution gets its own
+// independent state via ctx.tagState.
+func (node *tagIfchangedNode) getState(ctx *ExecutionContext) *ifchangedState {
+	if s, ok := ctx.tagState[node].(*ifchangedState); ok {
+		return s
+	}
+	s := &ifchangedState{}
+	ctx.tagState[node] = s
+	return s
+}
+
 func (node *tagIfchangedNode) Execute(ctx *ExecutionContext, writer TemplateWriter) *Error {
+	state := node.getState(ctx)
+
 	if len(node.watchedExpr) == 0 {
 		// Check against own rendered body
 
@@ -23,10 +41,21 @@ func (node *tagIfchangedNode) Execute(ctx *ExecutionContext, writer TemplateWrit
 		}
 
 		bufBytes := buf.Bytes()
-		if !bytes.Equal(node.lastContent, bufBytes) {
+
+		changed := !bytes.Equal(state.lastContent, bufBytes)
+		if changed {
+			state.lastContent = bufBytes
+		}
+
+		if changed {
 			// Rendered content changed, output it
 			writer.Write(bufBytes)
-			node.lastContent = bufBytes
+		} else if node.elseWrapper != nil {
+			// Content hasn't changed, render else block if present
+			err := node.elseWrapper.Execute(ctx, writer)
+			if err != nil {
+				return err
+			}
 		}
 	} else {
 		nowValues := make([]*Value, 0, len(node.watchedExpr))
@@ -39,16 +68,16 @@ func (node *tagIfchangedNode) Execute(ctx *ExecutionContext, writer TemplateWrit
 		}
 
 		// Compare old to new values now
-		changed := len(node.lastValues) == 0
+		changed := len(state.lastValues) == 0
 
-		for idx, oldVal := range node.lastValues {
+		for idx, oldVal := range state.lastValues {
 			if !oldVal.EqualValueTo(nowValues[idx]) {
 				changed = true
 				break // we can stop here because ONE value changed
 			}
 		}
 
-		node.lastValues = nowValues
+		state.lastValues = nowValues
 
 		if changed {
 			// Render thenWrapper