Skip to content

Commit

Permalink
Fix a regression introduced in cocotbgh-727 and cocotbgh-723, caused …
Browse files Browse the repository at this point in the history
…by a misunderstanding of how __new__ works

In those patches, I declared classes that were roughly
```python
class SomeClass:
    def __new__(cls, arg):
        try:
            return existing[arg]
        except KeyError:
            return super(SomeClass, cls).__new__(cls, arg)

    def __init__(self, arg):
        self.arg = arg
        self.state = 0
```

This approach has a fatal flaw (cocotbgh-729), with function calls shown in the following code:
```python
A = SomeClass(1)
B = SomeClass(1)
```

We need to override class-construction without allowing `__init__` to run a second time.

One option would be to just remove `__init__` entirely, and move the contents into `__new__`.
The other option, which I take in this patch, is to introduce a metaclass overriding the `__call__` operator on class types.

I'm not convinced this is the best approach, but it does fix the problem.
  • Loading branch information
eric-wieser committed Jan 7, 2019
1 parent 1c35b8d commit 1ebf47a
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 28 deletions.
40 changes: 12 additions & 28 deletions cocotb/triggers.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,10 @@
import simulator
from cocotb.log import SimLog
from cocotb.result import raise_error
from cocotb.utils import get_sim_steps, get_time_from_sim_steps
from cocotb.utils import (
get_sim_steps, get_time_from_sim_steps, with_metaclass,
ParametrizedSingleton
)


class TriggerException(Exception):
Expand Down Expand Up @@ -206,7 +209,7 @@ def NextTimeStep():
return _nxts


class _EdgeBase(GPITrigger):
class _EdgeBase(with_metaclass(ParametrizedSingleton, GPITrigger)):
"""
Execution will resume when an edge occurs on the provided signal
"""
Expand All @@ -218,19 +221,9 @@ def _edge_type(self):
"""
raise NotImplementedError

# Ensure that each signal has at most one edge trigger per edge type.
# Using a weak dictionary ensures we don't create a reference cycle
_instances = weakref.WeakValueDictionary()

def __new__(cls, signal):
# find the existing instance, if possible - else create a new one
key = (signal, cls._edge_type)
try:
return cls._instances[key]
except KeyError:
instance = super(_EdgeBase, cls).__new__(cls)
cls._instances[key] = instance
return instance
@classmethod
def __singleton_key__(cls, signal):
return signal

def __init__(self, signal):
super(_EdgeBase, self).__init__()
Expand Down Expand Up @@ -507,22 +500,13 @@ def prime(self, callback):
callback(self)


class Join(PythonTrigger):
class Join(with_metaclass(ParametrizedSingleton, PythonTrigger)):
"""
Join a coroutine, firing when it exits
"""
# Ensure that each coroutine has at most one join trigger.
# Using a weak dictionary ensures we don't create a reference cycle
_instances = weakref.WeakValueDictionary()

def __new__(cls, coroutine):
# find the existing instance, if possible - else create a new one
try:
return cls._instances[coroutine]
except KeyError:
instance = super(Join, cls).__new__(cls)
cls._instances[coroutine] = instance
return instance
@classmethod
def __singleton_key__(cls, coroutine):
return coroutine

def __init__(self, coroutine):
super(Join, self).__init__()
Expand Down
36 changes: 36 additions & 0 deletions cocotb/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import math
import os
import sys
import weakref

# For autodocumentation don't need the extension modules
if "SPHINX_BUILD" in os.environ:
Expand Down Expand Up @@ -412,6 +413,41 @@ def __prepare__(cls, name, this_bases):
return type.__new__(metaclass, 'temporary_class', (), {})


class ParametrizedSingleton(type):
"""
A metaclass that allows class construction to reuse an existing instance
We use this so that `RisingEdge(sig)` and `Join(coroutine)` always return
the same instance, rather than creating new copies.
"""

def __init__(cls, *args, **kwargs):
# Attach a lookup table to this class.
# Weak such that if the instance is no longer referenced, it can be
# collected.
cls.__instances = weakref.WeakValueDictionary()

def __singleton_key__(cls, *args, **kwargs):
"""
Convert the construction arguments into a normalized representation that
uniquely identifies this singleton.
"""
# Once we drop python 2, we can implement a default like the following,
# which will work in 99% of cases:
# return tuple(inspect.Signature(cls).bind(*args, **kwargs).arguments.items())
raise NotImplementedError

def __call__(cls, *args, **kwargs):
key = cls.__singleton_key__(*args, **kwargs)
try:
return cls.__instances[key]
except KeyError:
# construct the object as normal
self = super(ParametrizedSingleton, cls).__call__(*args, **kwargs)
cls.__instances[key] = self
return self


if __name__ == "__main__":
import random
a = ""
Expand Down
18 changes: 18 additions & 0 deletions tests/test_cases/test_cocotb/test_cocotb.py
Original file line number Diff line number Diff line change
Expand Up @@ -763,6 +763,24 @@ def test_join_identity(dut):
clk_gen.kill()


@cocotb.test()
def test_edge_identity(dut):
"""
Test that Edge triggers returns the same object each time
"""

re = RisingEdge(dut.clk)
fe = FallingEdge(dut.clk)
e = Edge(dut.clk)

assert re is RisingEdge(dut.clk)
assert fe is FallingEdge(dut.clk)
assert e is Edge(dut.clk)

# check they are all unique
assert len({re, fe, e}) == 3
yield Timer(1)


if sys.version_info[:2] >= (3, 3):
# this would be a syntax error in older python, so we do the whole
Expand Down

0 comments on commit 1ebf47a

Please sign in to comment.