Add ContextMaps to let users create various ContextMaps

[idelpivnitskiy]

Motivation:

If we plan to use ContextMap for more and more use-cases as a strictly typed variant of a Map, we need to open a way to create various versions of the ContextMap.

Modifications:

• Add ContextMaps utility class;
• Add SingletonContextMap and UnmodifiableContextMap;
• Move NoopAsyncContextProvider.NoopContextMap to EmptyContextMap;
• Move ConcurrentContextMap and CopyOnWriteContextMap to context-api;
• Copy io.servicetalk.concurrent.internal.DefaultContextMap to io.servicetalk.context.api.DefaultContextMap;
• Deprecate io.servicetalk.concurrent.internal.DefaultContextMap;
• Copy io.servicetalk.concurrent.internal.ContextMapUtils to io.servicetalk.context.api.ContextMapUtils;
• Deprecate io.servicetalk.concurrent.internal.ContextMapUtils;

Result:

New public API to create various ContextMaps.

[bryce-anderson]

I don't like the idea of introducing a new unmodifiable context map: it's not really the same notion as regular context map because all the set operations don't work and then all the places you currently consume context map you'll have to start wondering if it's going to throw if you try to modify it.

[idelpivnitskiy]

Is it not the same for Map? When you get a Map in any method you also have no idea if it's a modifiable or not, but it wasn't been a problem in most cases because the behavior is assumed based on the business logic needs.
ContextMap is not only for AsyncContext or request.context(). It's a general purpose map with strong types based on the Key. We expect to use it in other scenarios as needed.

Alternatively, we can keep the SDE.metadata() modifiable, similar to request.context(). That way SD filters will be able to adjust metadata before it reached LB.

The main purpose of this PR is to provide public API to create ContextMap, I can remove unmodifiableMap and singletonMap.

[bryce-anderson]

Is it not the same for Map? When you get a Map in any method you also have no idea if it's a modifiable or not, but it wasn't been a problem in most cases because the behavior is assumed based on the business logic needs.

It is the same for Map but that feels like it was a mistake as well and I see no reason to repeat it.

ContextMap is not only for AsyncContext or request.context(). It's a general purpose map with strong types based on the Key. We expect to use it in other scenarios as needed.

I appreciate the strong type extraction based on the key, but in the case of meta-data I don't like the strong instance based coupling that Key requires. Metadata is typically loosely coupled information that you may or may not care about while requiring the setter and consumer of that data use the exact same Key creates a much stronger coupling.

Alternatively, we can keep the SDE.metadata() modifiable, similar to request.context(). That way SD filters will be able to adjust metadata before it reached LB.

That gets us back to the difficulty of having a default ContextMap metadata() { .. } method to preserve the ServiceDiscovererEvent API. Maybe there is another way?

[idelpivnitskiy]

It is the same for Map but that feels like it was a mistake as well and I see no reason to repeat it.

I haven't seen many cases when it was a problem for the Map. Feels natural for any Collection.

I appreciate the strong type extraction based on the key, but in the case of meta-data I don't like the strong instance based coupling that Key requires. Metadata is typically loosely coupled information that you may or may not care about while requiring the setter and consumer of that data use the exact same Key creates a much stronger coupling.

That was a design decision for ContextMap, and I don't see why it's a problem to have an instance based Key. That helps to prevent users from accessing keys they should not have access to. For public keys, sharing an instance feels better that sharing a "name" that must be used by a string. Knowledge of a name is also a coupling, not much different from sharing a strongly typed instance.

That gets us back to the difficulty of having a default ContextMap metadata() { .. } method to preserve the ServiceDiscovererEvent API. Maybe there is another way?

Can you elaborate what is the difficulty?

[bryce-anderson]

Much of this is perhaps a moot point as I'm starting to think that maybe we don't need meta-data at all, but in case we change our minds here it is.

I haven't seen many cases when it was a problem for the Map. Feels natural for any Collection.

Fair enough. I've seen a number of cases where either trying to mutate threw exceptions or someone mutated a map they shouldn't have (someone forgot to make it unmodifiable) and it made something else sad (much harder to track down). Admittedly my opinion of it is largely influenced by Scala where there are mutable and immutable collections which made it really easy to reason about and prevent these types of problems at compile time. Java just doesn't have this in the standard library so I suppose people just have to get used to relying on convention instead of tooling.

That was a design decision for ContextMap, and I don't see why it's a problem to have an instance based Key. That helps to prevent users from accessing keys they should not have access to. For public keys, sharing an instance feels better that sharing a "name" that must be used by a string. Knowledge of a name is also a coupling, not much different from sharing a strongly typed instance.

To be clear, in general I really like the instance based coupling of abstractions like ContextMap for all the reasons you laid out. However, for meta-data specifically the information tends to be loose and more structural based so I just don't think that behavior is a good fit for meta-data.

Can you elaborate what is the difficulty?

For sure. Lets say we want to preserve the API of ServiceDiscovererEvent so we add a default method like such:

default ContextMap context() {
  // now what? From the interface we can't define state. The `ContextMap` is mutable so we can't
  // use a shared instance and we can't return a `new DefaultContextMap()` because if someone
  // tries to set something it will be lost.
}

Map<String, Object> suffers from this problem as well but as you noted people are already used to having some maps being unmodifiable so maybe they would look at the docs and we can state there that we return an unmodifiable map.

[idelpivnitskiy]

// now what? From the interface we can't define state. The ContextMap is mutable so we can't
// use a shared instance and we can't return a new DefaultContextMap() because if someone
// tries to set something it will be lost.

Depending on how we would like to define meta-data. If we say in its javadoc that it's immutable, then we return ContextMaps.emptyMap() by default. If there are any "filters" on the way that want to modify it, they will wrap SDE with another instance of ContextMap that has an altered view of the meta-data.
If we say it's modifiable, like request.context(), then we return ContextMaps.newDefaultMap(). If there are any fitters on the way for SDE, they will modify meta-data the same way HTTP request filters can.

[bryce-anderson]

If we say it's modifiable, like request.context(), then we return ContextMaps.newDefaultMap(). If there are any fitters on the way for SDE, they will modify meta-data the same way HTTP request filters can.

This wont work. Let's say there is a filter that wants to set a meta-data entry. When it calls metadata().put(key, value) it has no effect since the context map created by default ContextMap context() { return new ContextMaps.newDefaultMap() } will be promptly thrown away.

[idelpivnitskiy]

Hm, that's a good point. Then it's better to either use emptyMap() by default or throw UnsupportedOperationException from the interface. In DefaultServiceDiscoveryEvent, we can use newDefaultMap().