Immutable Context chain

[MindFlavor]

Intro

Following @JeffreyRichter's comment on PR #504 this PR proposes:

• Immutable Context. Each policy can add information to the Context without modifying the original Context.
• Policy's send function no longer accepts a mutable Context instance (because they are immutable).
• Removal of PipelineContext (obsoleted by the new Context type map).
• Policy's send function no longer accepts a PipelineContext<T> but a much simpler Context.

Immutable Context

The Context must be immutable in the sense that policies can add information "overriding" the preexisting one. This is done by implementing a Context stack with topmost winner policy. In other words, the final type map is the union of every type map in the stack, picking only the topmost entity in case of collisions.

Policies can change the contents of a received Context by wrapping it with the provided function. The new Context can then by defined as mutable. Every operation on the new Context does not change the underlying one(s).

In order to keep the original Context clonable but to prevent the cloning of the overidden Context, I've introduced another structure OverridableContext that performs the "type overriding" function without cloning. For efficiency the wrapping structure only gets a reference of the wrapped one (with guaranteed lifetime match since we are strictly walking the stack).

Since we are talking about "stack based" override, to get the previous value you just need to drop the overriding context (or let it go out of scope).

See this example taken by the added test case. Here we are simulating two pipeline policies. The first Context is provided by the SDK user (via Context::new()). The following policies can override the value by simply calling context.create_override(). The struct will take care of everything else:

 #[test]
  fn overriding_two_levels() {
      let mut context = Context::new();
      context.insert_or_replace(0u8);
      assert_eq!(0, *context.get::<u8>().unwrap());

      {
          // level 1
          let mut context = context.create_override();
          context.insert_or_replace(1u8);
          assert_eq!(1, *context.get::<u8>().unwrap());

          {
              // level 2
              let mut context = context.create_override();
              context.insert_or_replace(2u8);
              assert_eq!(2, *context.get::<u8>().unwrap());
          }

          assert_eq!(1, *context.get::<u8>().unwrap());
      }

      assert_eq!(0, *context.get::<u8>().unwrap());
  }

Removal of PipelineContext (obsoleted by the new Context type map).

This changes removes a lot of useless - and confusing - code. Now the Context type map is able to store arbitrary data so the PipelineContext is no longer needed.

See the Data Lake code, is a lot leaner as a result.

[rylev]

I'm not sure I 100% understand the utility of an immutable Context, but if that's the way we want to go, this implementation looks pretty good. I had some nits.

[MindFlavor]

I've changed the logic to get rid of that pesky Arc. Additionally, I've shifted from owned structs to simple references.
This means quicker code and also no need of "de-structure" the Context manually to access the overridden one.

[thovoll]

This reads like creating an override without actually overriding anything.

[MindFlavor]

Good point. It's an unpleasant side effect of being unable to transform Context into a trait object 😢. If it were possible, send would accept it instead of a concrete OverridableContext instance.

I'll try to come up with something better!

[MindFlavor]

@thovoll: what do you think now? I've replaced create_override() with into() when you do not want to override anything.

[thovoll]

Thanks @MindFlavor, looks better to me.

[yoshuawuyts]

I feel like this may not be the right direction for us to go at this moment. Unlike Golang or C# the Context type is scoped only to our SDK, and is not a shared construct among libraries. The main case we need to cover for is being able to construct a Context instance once, and copy standalone instances of it that we can hand out to individual requests. We already support this through our #[derive(Clone)] Context {} implementation.

Within the pipeline construct of the SDK, the point of having a Context is that to enable shared mutable state on a per-request basis. Clone-on-Write semantics don't make much sense within a request pipeline, and if I'm reading the current implementation right, might even lead to performance degradation. I agree we should revisit the relationship between Pipeline and Context, but I believe there may be alternative ways of achieving similar results.

I feel like I've mentioned this before, but I would prefer it if we gained understanding the full range of uses of it before we propose any further changes to it. As I mentioned in #504, the final question we appear to need to answer about Context usage is whether it needs to interact with (distributed) tracing. I've added that to the agenda for tomorrow's meeting.

So to summarize: I think we should hold off on deciding whether we want to take Context in this direction until we have more information on how Context and distributed tracing interact.

[MindFlavor]

Within the pipeline construct of the SDK, the point of having a Context is that to enable _shared mutable state on a per-request basis.

I do not think it's the whole story. Quoting @JeffreyRichter's comment (in #504 (comment)):

But, I think we should make some other changes. Context is supposed to work like the call graph. That is, some code at the top creates a Context and adds a value to it. Then it passes it to some other function which may add another value to it, etc. As the stack unwinds, the added values should automatically be removed.

That is exactly what this PR provides: a stack based type map. Each policy in the pipeline can add a value and it will get "propagated" to the following policies but it will get "removed" as soon as the stack unwinds.

A simple shared mutable state won't work because any change would be irreversible.

Clone-on-Write semantics don't make much sense within a request pipeline, and if I'm reading the current implementation right, might even lead to performance degradation.

The Cow here is simply a way to either store a Context or its reference (it's up to the caller). It will never be cloned nor mutated. Also a policy that does not want to "add information" to the context can simply pass down the same reference as before with no performance impact whatsoever.
In origin, I had only a &Context reference instead of Cow but @thovoll's comment prompted me to this change. If you don't like it we can easily remove it (as I said the Context is never cloned and we only need a non mutable ref to it for the code to work).

I feel like I've mentioned this before, but I would prefer it if we gained understanding the full range of uses of it before we propose any further changes to it. As I mentioned in #504, the final question we appear to need to answer about Context usage is whether it needs to interact with (distributed) tracing. I've added that to the agenda for tomorrow's meeting.

I agree. I am translating into code the ideas we are discussing because I think it's easier to reason on pros and cons of an approach while reading real, working code. Also, I want this discussion to be in the open: this is an important design decision that could benefit from every input and should remain "documented" for posterity.

[rylev]

I agree with @yoshuawuyts that we probably want to make sure we fully understand the use case for Context before we decide on which direction to go with, but the implementation here got much more complicated than it was last time I reviewed, and I'm not convinced it needed to. There's now two Context types again and a trait to unify them, and I'm unsure why.

[rylev]

I don't really get why we need two types Context and OverrideableContext. Is this just to ensure that one is immutable and the other isn't? Wouldn't the fact that the send method takes an immutable reference guarantee that?

[rylev]

Why is this trait needed? Is this just to ensure that Context and OverrideableContext have the same methods? We don't seem to ever require that in code and so this feels like it just complicates things.

[rylev]

Using Context for something that's required feels wrong. It would be nice if this remained something that is checked at compile time.

[MindFlavor]

I'm going to close this and reopen a new PR following a discussion with @JeffreyRichter.

Key points:

1. Pipeline must not mutate the Context.
2. A Context should be "enrichable" to allow SDK users to add information on specific cases. Other SDKs keep a context tree for performance reasons and for cancellation. Since we do not use Context for cancellation we can get away with a simple clone as long as it's cheap.