[monitorlib, mock_uss] Improve SynchronizedValue by BenjaminPelletier · Pull Request #1207 · interuss/monitoring

BenjaminPelletier · 2025-10-03T20:48:19Z

In a number of places, we synchronize data across multiple threads using SynchronizedValue. This tool ensures read-modify-write transactions are not interrupted by changes from another thread. Currently, however, the __enter__/__exit__ pattern used does not provide useful type-hinting for IDEs nor for correctness checks like ruff/basedpyright, and it also prevents certain operations like replacing content rather than mutating it during a transaction. This PR attempts to address these shortcomings by adding a transact() method to SynchronizedValue so that usage is now with db.transact() as tx: rather than the more confusing and less inspectable with db as tx:.

The old behavior is retained as deprecation-annotated methods to avoid making this PR huge by changing all usages at once. However, the flights storage of mock_uss is entirely switched to the new usage to verify functionality.

A number of ruff/basedpyright errors revealed by the improved inspectability of SynchronizedValue types are fixed, though one is added; see comment below.

BenjaminPelletier · 2025-10-03T20:56:53Z

There is one code style finding that puzzles me. The error is:

...which refers to this code:

I assume the problem is that flights[flight_id] can be either a FlightRecord or None:

However, on line 43 we are sure flights[flight_id] is not None because we just checked that on line 40. So, it seems we definitely know we have a FlightRecord and not None, yet the checker complains (IIUC) that flights[flight_id].locked isn't valid since flights[flight_id] might be None, and None doesn't have a locked field.

@the-glu , do you have any ideas on this?

the-glu · 2025-10-06T10:05:51Z

+            return flask.jsonify(response), 412
+        logger.info(f"Deleted ISA {deleted_isa.dss_query.isa.id}")
+    else:
+        deleted_isa = None


Nit: I would have the shorted branch on top of the if, to be able have everything close (here you need to backtrack almost 20 lines to check the condition again)

the-glu · 2025-10-06T10:06:15Z

+    else:
+        deleted_isa = None
+
    result = ChangeTestResponse(version=record.version, injected_flights=record.flights)


nit: I would move result on line 137 and merge both if (138/158)

the-glu · 2025-10-06T10:08:31Z

+
+    def _unlock(self, exc_type=None, exc_val=None, exc_tb=None):
+        self._lock.__exit__(exc_type, exc_val, exc_tb)
+        self._locked = False


Should _value also be set to None for consistency?

Yes, I think so -- thanks

the-glu · 2025-10-06T10:10:04Z


+    def transact(self) -> Transaction[TValue]:
+        return Transaction[TValue](
+            self._lock, lambda: self._get_value(), lambda v: self._set_value(v)


Are lambdas needed? They don't seems to add anything ^^'

No; removed -- thanks

the-glu · 2025-10-06T10:11:50Z

-        self._current_value = self._get_value()
-        return self._current_value
+        if self._transaction:
+            raise RuntimeError(


Wouldn't it be nice to have it on the new __enter__ as well ?

Yes; added check for self._locked

the-glu · 2025-10-06T10:15:27Z

There is one code style finding that puzzles me.
@the-glu , do you have any ideas on this?

I tried to debug locally on your branch, but basedpyright didn't gave me any error, was there some caching somewhere?

BenjaminPelletier · 2025-10-06T20:31:56Z

There is one code style finding that puzzles me.
@the-glu , do you have any ideas on this?

I tried to debug locally on your branch, but basedpyright didn't gave me any error, was there some caching somewhere?

I added the error to the baseline for this PR, but I don't think I should have had to do that.

the-glu · 2025-10-07T10:00:36Z

I added the error to the baseline for this PR, but I don't think I should have had to do that.

Ho, no worries, I disabled it for testing.

I think the linter is complaining because he cannot be sure .value[flight_id] will be the same between the two calls, the getter may as well return a random value in that case :)

Switching to

            f = tx.value.flights[flight_id]

            if f:
                # FlightRecord was a true existing flight
                log(f"Releasing lock on existing flight_id {flight_id}")
                f.locked = False

Make it happy :)

(The whole variable is needed, even storing just tx.value.flights raise the error, so I'm not sure my explanation is right. Maybe it doesn't know it's a lock and assume another thread my edit the dict between the two lines?)

the-glu · 2025-10-07T10:03:03Z

(The whole variable is needed, even storing just tx.value.flights raise the error, so I'm not sure my explanation is right. Maybe it doesn't know it's a lock and assume another thread my edit the dict between the two lines?)

Based on microsoft/pyright#703 (comment) , it's indeed treating [x] as having possible different values between the two lines :)

BenjaminPelletier · 2025-10-08T05:39:07Z

I think the linter is complaining because he cannot be sure .value[flight_id] will be the same between the two calls, the getter may as well return a random value in that case :)

Ah, I think that makes some sense -- thanks. It is interesting what the type checker thinks can be assumed and what can't, since, e.g., field access (foo.bar) could be overridden to return a random value also, but it seems like the type checker recognizes that checking a field value (to be, e.g., not None) means that field value can be assumed to retain that property later on.

BenjaminPelletier · 2025-10-08T06:10:10Z

@barroco or @mickmis I think this is now ready for Committer review

the-glu · 2025-10-08T06:55:23Z

Ah, I think that makes some sense -- thanks. It is interesting what the type checker thinks can be assumed and what can't, since, e.g., field access (foo.bar) could be overridden to return a random value also, but it seems like the type checker recognizes that checking a field value (to be, e.g., not None) means that field value can be assumed to retain that property later on.

Yes, behind the scene it does use some patterns, and with type narrowing we can also use it to help typing, should it make sense at some point, e.g. in implicitdict :)

mickmis

LGTM with some nits and modulo clarification of checks for value being None.

mickmis · 2025-10-08T09:00:14Z

-            )
-            result["query"] = notification.query
+
+    if record.isa_version is not None:


Checking ridsp_create_test, isa_version is optional only because we receive the ISA version after creating the record. However when accessing it from ridsp_delete_test it should never happen that isa_version is None.

Here assert instead that it is not None by raising an exception if it is? Or alternatively remove the optional on isa_version? And/or create the record only after the ISA was successfully created?

I think it probably happens that the current implementation may ensure isa_version is not None, but I don't think that is a guaranteed behavior from the perspective of the deletion handler. In the deletion handler, the question is what to do if we have (for whatever reason) a flight record with an isa_version that is None. The type annotation of isa_version explicitly allows for it to be None, so the deletion handler should handle that situation as gracefully as possible. Since it is definitely possible to delete a flight even if an ISA wasn't emplaced in a DSS, it seems like this check is what we would want to do.

We could remove the optional on isa_version, consistently guaranteeing that isa_version is not None at this point, which would have impacts in multiple places including this deletion handler. But, 1) I don't think that change would belong in this PR since that would be a behavioral/expectations change in mock_uss which doesn't relate to SynchronizedValue and its transactions, and 2) I could imagine some circumstances in which we may want to enhance mock_uss to create flights even when it can't or doesn't create an ISA in a DSS (e.g., off-nominal behavior when a DSS is down). I'm hoping to replace RID injection with calls to the flight_planning API when the flight_planning API can specify UA positions in which case there would presumably often be cases when a flight record has no isa_version.

Since it is definitely possible to delete a flight even if an ISA wasn't emplaced in a DSS, it seems like this check is what we would want to do.

Indeed!

mickmis · 2025-10-08T09:04:36Z

+
+
+# Note: attempts to change the below to SynchronizedValue[TValue] causes problems because IntelliJ does not reliably
+# understand the newer syntax and therefore fails to provide contextual information for specific TValues.


nit: seems to be a known issue: https://docs.astral.sh/ruff/rules/non-pep695-generic-class/#known-problems
Link this?

mickmis · 2025-10-08T09:09:29Z

+    @property
+    def value(self) -> TValue:
+        """Value exposed for this transaction.  Mutate or set it to make changes during the transaction."""
+        if not self._locked or self._value is None:


Does this imply that self_value may never be None? What if TValue is Something | None? I don't see this scenario prevented by something.

Also, I'm not sure I understand the intent behind checking for self._value is None: is it to ensure that a write has been performed before? Or to handle a race with a concurrent thread being between lines 40 & 41?

That's fair -- my implicit assumption was that TValue would always be a non-Noneable concrete value, so value=None could be used to communicate absence of value. But, thinking about things a little more, it doesn't seem like that's an important assumption -- I'll adjust to make _value a pure TValue (rather than TValue | None) and I think that will take care of both this comment and the following one.

mickmis · 2025-10-08T09:23:01Z

+        if not self._locked:
+            return
+        try:
+            if self.value is not None and exc_type is None:


Same as above: value being None is not a valid scenario?

Agreed and adjusted

Improve SynchronizedValue

f627494

BenjaminPelletier marked this pull request as ready for review October 3, 2025 20:57

the-glu reviewed Oct 6, 2025

View reviewed changes

Address comments

b226f32

BenjaminPelletier added 3 commits October 8, 2025 05:39

Fix type checker's skepticism

4a69e2b

Merge remote-tracking branch 'interuss/main' into synchronized-value

8b8ba94

Merge remote-tracking branch 'interuss/main' into synchronized-value

089c6e1

mickmis self-assigned this Oct 8, 2025

mickmis approved these changes Oct 8, 2025

View reviewed changes

BenjaminPelletier added 2 commits October 8, 2025 17:07

Address @mickmis comments

4b4c312

Merge remote-tracking branch 'interuss/main' into synchronized-value

4803f7f

BenjaminPelletier merged commit d4c6d5f into interuss:main Oct 8, 2025
21 checks passed

BenjaminPelletier deleted the synchronized-value branch October 8, 2025 17:17

github-actions Bot added a commit that referenced this pull request Oct 8, 2025

[monitorlib, mock_uss] Improve SynchronizedValue (#1207) d4c6d5f

dd409f7

BenjaminPelletier mentioned this pull request Oct 10, 2025

[monitorlib] Upgrade remaining SynchronizedValue usages #1217

Merged



		# Note: attempts to change the below to SynchronizedValue[TValue] causes problems because IntelliJ does not reliably
		# understand the newer syntax and therefore fails to provide contextual information for specific TValues.

Conversation

BenjaminPelletier commented Oct 3, 2025

Uh oh!

BenjaminPelletier commented Oct 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

the-glu commented Oct 6, 2025

Uh oh!

BenjaminPelletier commented Oct 6, 2025

Uh oh!

the-glu commented Oct 7, 2025

Uh oh!

the-glu commented Oct 7, 2025

Uh oh!

BenjaminPelletier commented Oct 8, 2025

Uh oh!

BenjaminPelletier commented Oct 8, 2025

Uh oh!

the-glu commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mickmis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

the-glu commented Oct 8, 2025 •

edited

Loading