Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix typechecking with twisted trunk #16121

Merged
merged 8 commits into from
Aug 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/twisted_trunk.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ jobs:
poetry remove twisted
poetry add --extras tls git+https://github.com/twisted/twisted.git#${{ inputs.twisted_ref || 'trunk' }}
poetry install --no-interaction --extras "all test"
- name: Remove warn_unused_ignores from mypy config
run: sed '/warn_unused_ignores = True/d' -i mypy.ini
- name: Remove unhelpful options from mypy config
run: sed -e '/warn_unused_ignores = True/d' -e '/warn_redundant_casts = True/d' -i mypy.ini
- run: poetry run mypy

trial:
Expand Down
1 change: 1 addition & 0 deletions changelog.d/16121.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Attempt to fix the twisted trunk job.
32 changes: 16 additions & 16 deletions synapse/handlers/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -1474,23 +1474,23 @@ async def handle_new_client_event(

# We now persist the event (and update the cache in parallel, since we
# don't want to block on it).
event, context = events_and_context[0]
clokep marked this conversation as resolved.
Show resolved Hide resolved
#
# Note: mypy gets confused if we inline dl and check with twisted#11770.
# Some kind of bug in mypy's deduction?
deferreds = (
run_in_background(
self._persist_events,
requester=requester,
events_and_context=events_and_context,
ratelimit=ratelimit,
extra_users=extra_users,
),
run_in_background(
self.cache_joined_hosts_for_events, events_and_context
).addErrback(log_failure, "cache_joined_hosts_for_event failed"),
)
result, _ = await make_deferred_yieldable(
gather_results(
(
run_in_background(
self._persist_events,
requester=requester,
events_and_context=events_and_context,
ratelimit=ratelimit,
extra_users=extra_users,
),
run_in_background(
self.cache_joined_hosts_for_events, events_and_context
).addErrback(log_failure, "cache_joined_hosts_for_event failed"),
),
consumeErrors=True,
)
gather_results(deferreds, consumeErrors=True)
).addErrback(unwrapFirstError)

return result
Expand Down
19 changes: 10 additions & 9 deletions synapse/logging/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -809,23 +809,24 @@ def run_in_background( # type: ignore[misc]

# `res` may be a coroutine, `Deferred`, some other kind of awaitable, or a plain
# value. Convert it to a `Deferred`.
d: "defer.Deferred[R]"
if isinstance(res, typing.Coroutine):
# Wrap the coroutine in a `Deferred`.
res = defer.ensureDeferred(res)
d = defer.ensureDeferred(res)
elif isinstance(res, defer.Deferred):
pass
d = res
elif isinstance(res, Awaitable):
# `res` is probably some kind of completed awaitable, such as a `DoneAwaitable`
# or `Future` from `make_awaitable`.
res = defer.ensureDeferred(_unwrap_awaitable(res))
d = defer.ensureDeferred(_unwrap_awaitable(res))
else:
# `res` is a plain value. Wrap it in a `Deferred`.
res = defer.succeed(res)
d = defer.succeed(res)

if res.called and not res.paused:
if d.called and not d.paused:
# The function should have maintained the logcontext, so we can
# optimise out the messing about
return res
return d

# The function may have reset the context before returning, so
# we need to restore it now.
Expand All @@ -843,8 +844,8 @@ def run_in_background( # type: ignore[misc]
# which is supposed to have a single entry and exit point. But
# by spawning off another deferred, we are effectively
# adding a new exit point.)
res.addBoth(_set_context_cb, ctx)
return res
d.addBoth(_set_context_cb, ctx)
return d


T = TypeVar("T")
Expand Down Expand Up @@ -877,7 +878,7 @@ def make_deferred_yieldable(deferred: "defer.Deferred[T]") -> "defer.Deferred[T]
ResultT = TypeVar("ResultT")


def _set_context_cb(result: ResultT, context: LoggingContext) -> ResultT:
def _set_context_cb(result: ResultT, context: LoggingContextOrSentinel) -> ResultT:
"""A callback function which just sets the logging context"""
set_current_context(context)
return result
Expand Down
2 changes: 1 addition & 1 deletion synapse/util/caches/deferred_cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ def __init__(self) -> None:
def deferred(self, key: KT) -> "defer.Deferred[VT]":
if not self._deferred:
self._deferred = ObservableDeferred(defer.Deferred(), consumeErrors=True)
return self._deferred.observe().addCallback(lambda res: res.get(key))
return self._deferred.observe().addCallback(lambda res: res[key])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the point is that this lookup never fails---but please double check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very not-confident about this change. I think you're correct, but I'm very lost in how this is used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. I took another look at this and I agree that if this is not true then the internal machinery is broken. It'll raise errors anyway, so I think this is OK. (Otherwise you'd get back an unexpected None, which seems worse TBH)


def add_invalidation_callback(
self, key: KT, callback: Optional[Callable[[], None]]
Expand Down
14 changes: 6 additions & 8 deletions tests/util/test_async_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,9 @@ def check_called_first(res: int) -> int:
observer1.addBoth(check_called_first)

# store the results
results: List[Optional[ObservableDeferred[int]]] = [None, None]
results: List[Optional[int]] = [None, None]

def check_val(
res: ObservableDeferred[int], idx: int
) -> ObservableDeferred[int]:
def check_val(res: int, idx: int) -> int:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this was correct by running the test with print(type(res)) inserted.

results[idx] = res
return res

Expand Down Expand Up @@ -93,14 +91,14 @@ def check_called_first(res: int) -> int:
observer1.addBoth(check_called_first)

# store the results
results: List[Optional[ObservableDeferred[str]]] = [None, None]
results: List[Optional[Failure]] = [None, None]

def check_val(res: ObservableDeferred[str], idx: int) -> None:
def check_failure(res: Failure, idx: int) -> None:
results[idx] = res
return None

observer1.addErrback(check_val, 0)
observer2.addErrback(check_val, 1)
observer1.addErrback(check_failure, 0)
observer2.addErrback(check_failure, 1)

try:
raise Exception("gah!")
Expand Down
Loading