open-telemetry / open-telemetry/opentelemetry-php

Context is not actually immutable

Open
#2,044 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.