open-telemetry / open-telemetry/opentelemetry-php
Context is not actually immutable
Nobody has claimed this yet.
- Dominant language
- PHP
- Stars
- 912
- Forks
- 232
- Avg merge
- 7d 16h
- Merged PRs (30d)
- 4
Description
Steps to reproduce
The specification for context: https://github.com/open-telemetry/opentelemetry-specification/tree/v1.44.0/specification/context
Requires a Context instance to be strictly immutable. The current implementation fails this promise.
What is the expected behavior?
Any supported value in Context::with() should either be deeply cloned, or immutable.
What is the actual behavior?
#[\Override]
public function with(ContextKeyInterface $key, $value): self
{
if ($this->get($key) === $value) {
return $this;
}
$self = clone $this;
if ($key === self::$spanContextKey) {
$self->span = $value; // @phan-suppress-current-line PhanTypeMismatchPropertyReal
return $self;
}
$id = spl_object_id($key);
if ($value !== null) {
$self->context[$id] = $value;
$self->contextKeys[$id] ??= $key;
} else {
unset(
$self->context[$id],
$self->contextKeys[$id],
);
}
return $self;
}
On clone the array is cloned, but the values are copied. Any object typed values are therefore not cloned.
This means that storing a mutable object could lead to unexpected behavior:
$context = Context::getCurrent()
$key = Context::createKey("test");
$mutableValue = new ArrayObject(['message' => 'hello world']);
$context1 = $context->with($key, $mutableValue);
echo $context1->get($key)['message']; // hello world
$mutableValue['message'] = 'bye';
echo $context1->get($key)['message']; // bye
$key2 = Context::createKey("test2");
$context2 = $context->with($key2, 'test123');
echo $context2->get($key)['message']; // bye
/**
* meanwhile in another Fiber or any other part of the code:
*/
$mutableValue['message'] = 'hello world';
echo $context2->get($key)['message']; // hello world
While I believe that case one could be functioning as intended, I believe case 2 is definitely not what people will expect.
I believe this is not technically solvable in PHP.
If we define a Context to be a bag of keys and values, where keys are opaque identifiers (ContextKey), one could argue that we maybe should just not support non-scalar values.
The least we should do is put a comment in ContextInterface to explain the limitations and deviations from the spec (if you consider this a deviation).
Additional context
This is more of a theoretical problem than a practical one at this point.
Tip: React with 👍 to help prioritize this issue. Please use comments to provide useful context, avoiding +1 or me too, to help us triage it. Learn more here.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with Context::with() and ContextInterface, then compare their behavior with the linked OpenTelemetry Context specification. Determine whether supported values should be restricted, cloned, or documented as a limitation; done means the project has an agreed behavior and corresponding implementation or explanation.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- php
- Domain
- backend-api-design
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100