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 6 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.
28 changes: 15 additions & 13 deletions synapse/handlers/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -1474,21 +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?
dl = (
Copy link
Member

Choose a reason for hiding this comment

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

I don't get what dl stands for, can we use a longer variable name?

Copy link
Member

Choose a reason for hiding this comment

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

I think it is "deferred list"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, deferred list.

Thanks for getting this over the line.

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"),
),
dl,
consumeErrors=True,
)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove the comma after consumerErrors so black folds this a bit tighter?

).addErrback(unwrapFirstError)
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