Allow immutable borrow to access QuantumCircuit.parameters (backport #12918)

[mergify]

Summary

QuantumCircuit.parameters is logically a read-only operation on QuantumCircuit. For efficiency in multiple calls to assign_parameters, we actually cache the sort order of the internal ParameterTable on access. This is purely a caching effect, and should not leak out to users.

The previous implementation took a Rust-space mutable borrow out in order to (potentially) mutate the cache. This caused problems if multiple Python threads attempted to call assign_parameters simultaneously; it was possible for one thread to give up the GIL during its initial call to CircuitData::copy (so an immutable borrow was still live), allowing another thread to continue on to the getter CircuitData::get_parameters, which required a mutable borrow, which failed due to the paused thread in copy.

This moves the cache into a RefCell, allowing the parameter getters to take an immutable borrow as the receiver. We now write the cache out only if we can take the mutable borrow out necessary. This can mean that other threads will have to repeat the work of re-sorting the parameters, because their borrows were blocking the saving of the cache, but this will not cause failures.

The methods on ParameterTable that invalidate the cache all require a mutable borrow on the table itself. This makes it impossible for an immutable borrow to exist simultaneously on the cache, so these methods should always succeed to acquire the cache lock to invalidate it.

Details and comments

This won't be the last time we have problems with Python threading letting people see Rust reject conditions that allow data races. This particular case isn't the user's fault, though - QuantumCircuit.parameters shouldn't need a mutable borrow, and should be thread-safe to be combined with other thread-safe methods on QuantumCircuit. The issue was an internal caching detail, and that's something Qiskit should help.

An example script that produced errors before:

from concurrent.futures import ThreadPoolExecutor
from qiskit.circuit import QuantumCircuit

qc = QuantumCircuit(1, 1)
for _ in [None]*100_000:
    qc.measure(0, 0)

workers = 2

with ThreadPoolExecutor(max_workers=workers) as executor:
    fs = [executor.submit(qc.assign_parameters, []) for _ in [None]*workers]
    for future in fs:
        future.result()

The thread workers in this example aren't doing anything wrong; assign_parameters(inplace=False) should be safe to run in multiple threads. Before the Rust-space work, Python would silently ignore the potential data race, and the GIL would have prevented anything catastrophic happening (though multiple threads might have separately calculated the sort order).

In that example, the QuantumCircuit.copy call within QuantumCircuit.assign_parameters takes an immutable borrow in Rust space, but the Rust space method can become interrupted part way through, while copying the Measure instructions (since this requires yielding control to the Python interpreter to run Measure.copy). This allows another thread to progress to QuantumCircuit.parameters, which needed a mutable borrow, which could not be taken out due to the other paused thread.

---

This is an automatic backport of pull request #12918 done by Mergify.

[qiskit-bot]

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the following people are relevant to this code:

• @Qiskit/terra-core
• @kevinhartman
• @mtreinish

[coveralls]

Pull Request Test Coverage Report for Build 10391267961

Details

• 50 of 53 (94.34%) changed or added relevant lines in 2 files are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.009%) to 89.938%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/parameter_table.rs	49	52	94.23%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	2	92.98%

Totals
Change from base Build 10388036193:	0.009%
Covered Lines:	66734
Relevant Lines:	74200

---

💛 - Coveralls