AttachContext is not transitive

[rantav]

I realized there is another problem with AttachContext and friends.
Attaching a context and then creating a new context and attaching it, makes RequestCtx forget the previous attached context, which to me seems like a breach of the context contract

Example test:

func Test_ValueTransitive(t *testing.T) {
	ctx := acquireRequestCtx(new(fasthttp.RequestCtx))

	ctx2 := context.WithValue(ctx, "k2", "v2")
	ctx.AttachContext(ctx2)
	if v := ctx.Value("k2"); v != "v2" {
		t.Errorf("Value() key '%s' == %v, want %v", "k2", v, "v2")
	}

	ctx3 := context.WithValue(ctx, "k3", "v3")
	ctx.AttachContext(ctx3)
	// Should remember *both* k2 and k3
	if v := ctx.Value("k2"); v != "v2" {
		t.Errorf("Value() key '%s' == %v, want %v", "k2", v, "v2") // THIS FAILS, but should not
	}
	if v := ctx.Value("k3"); v != "v3" {
		t.Errorf("Value() key '%s' == %v, want %v", "k3", v, "v3")
	}
}

I'll send a pull request with fix suggestion soon

[savsgio]

Hi @rantav,

If you create a second context with value, you must use ctx.AttachedCtx() not the RequestCtx again, because fasthttp doesn't support overwrite the original RequestCtx. Atreugo only adds an extra layer to support attached context.

So must do:

// First time
ctx2 := context.WithValue(ctx, "k2", "v2")
ctx.AttachContext(ctx2)

// Second time or more
ctx3 :=  context.WithValue(ctx.AttachedContext(), "k3", "v3")
ctx.AttachContext(ctx3)

[rantav]

Thanks @savsgio I understand your point. Let me try to add another angle if I may.
Two points I think needs to be considered.

1. It's easy to miss the "use ctx.AttachedContext() instead of ctx" instruction. There's no compiler help and by convention everyone using a context.Context are already wrapping one context inside another so that would be their first instinct, to simply use ctx to attach a new value. So in terms of API ergonomics this could be a potential pitfall.
2. What if I attach a value to ctx and I also attach another value to actx. Now I want to add a third value, preserving the last two? a) I can't do that and b) there's a surprise element. Example:

ctx := acquireRequestCtx(new(fasthttp.RequestCtx))
ctx.SetUserValue("k1", "v1") // ctx contains k1
ctx2 := context.WithValue(ctx, "k2", "v2")
ctx.AttachContext(ctx2) // ctx contains k1, k2

// I want ctx3 to wrap both k1 and k2, but I can't. 
// Option 1
ctx3 := context.WithValue(ctx, "k3", "v3") // ctx3 contains k3, k1 and k2
// but if we do that: surprise 
ctx.AttachContext(ctx3) // now ctx contains k1, k3 (not k2) 

// Option 2 (instead of 1) 
ctx3 := context.WithValue(ctx2, "k3", "v3") // ctx3 contains k3, k2
// And now, surprise
ctx.AttachContext(ctx3) // now ctx contains k1, k3 (not k2) 

[savsgio]

I'm sorry, but your example is good, the ctx has the 3 keys:

fmt.Println(ctx.Value("k1"), ctx.Value("k2"), ctx.Value("k3"))
# Print: v1 v2 v3

Keep in mind, that you could only attach one context, if you attach another, you override the previous, that's why to use the ctx.AttachedContext() to keep the previously.
About use always the ctx and not the ctx.AttachedContext(), fasthttp and atreugo only implement the interface of context.Context to use as a simple context but not as a context with values, because it's generate some garbage and reduce the performance significally.

Maybe in a future, @erikdubbelboer or @kirillDanshin will implement it, but now, it's not possible.

[rantav]

ok, I get it, it's a limitation or "by design" of fasthttp.

[savsgio]

Sorry, use attached context carefully.
I could't say you anything more!