Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

room power-levels should fail gracefully when Synapse not reachable #164

Open
JOJ0 opened this issue Dec 5, 2024 · 3 comments · May be fixed by #168
Open

room power-levels should fail gracefully when Synapse not reachable #164

JOJ0 opened this issue Dec 5, 2024 · 3 comments · May be fixed by #168
Labels
bug Something isn't working

Comments

@JOJ0
Copy link
Owner

JOJ0 commented Dec 5, 2024 

$ synadm room power-levels
ERROR ConnectionError while querying Synapse: HTTPConnectionPool(host='localhost', port=8008): Max retries exceeded with url: /_synapse/admin/v1/rooms?from=0&limit=10 (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x10405aa90>: Failed to establish a new connection: [Errno 61] Connection refused'))
...
...

  File "/Users/user/git/synadm/synadm/api.py", line 849, in room_power_levels
    for i, room in enumerate(rooms["rooms"]):
                             ~~~~~^^^^^^^^^
TypeError: 'NoneType' object is not subscriptable
@JOJ0 JOJ0 added bug Something isn't working good first issue Good for newcomers labels Dec 5, 2024
JacksonChen666 added a commit that referenced this issue Jan 22, 2025
@JacksonChen666
hack fix #164
993b172 
References: #164
@JacksonChen666 JacksonChen666 removed the good first issue Good for newcomers label Jan 22, 2025
@JacksonChen666
Copy link
Collaborator

I've removed the "good first issue" label because glancing over how stuff is handled, it doesn't seem easy. The easiest thing is adding conditions to check if it's None, but that's code that'll be repeated everywhere. The API code also queries Synapse for more stuff in a loop, and then that gets trickier to handle because we have to handle retries now (everywhere as well, also would not recommend) and it's multiple requests in a single "API" function as well.

Retries are already handled apparently, just from reading your logs:

ERROR ConnectionError while querying Synapse: HTTPConnectionPool(host='localhost', port=8008): Max retries exceeded with url: /_synapse/admin/v1/rooms?from=0&limit=10 (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x10405aa90>: Failed to establish a new connection: [Errno 61] Connection refused'))

It's just that we don't have a nice way of displaying requests errors and complain (I think, could we do fatal logging?), and instead the query function just returns None. Maybe the query function should throw an exception but I'm not sure if that's good API design (throwing exceptions).

Also should point out that the rest of the codebase has a similar issue with failed requests handling: It doesn't. Here's an output from npx pyright (yes, a Python type checker in JS. I'm aware 🙃):

/Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:329:12 - error: Operator "in" not supported for types "Literal['room_id']" and "Any | None"
    Operator "in" not supported for types "Literal['room_id']" and "None" (reportOperatorIssue)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:330:20 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:473:30 - error: "get" is not a known attribute of "None" (reportOptionalMemberAccess)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:819:30 - error: "get" is not a known attribute of "None" (reportOptionalMemberAccess)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:866:34 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:867:13 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:869:22 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:875:25 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:879:25 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:889:26 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:891:9 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:1069:23 - error: "before_ts" is possibly unbound (reportPossiblyUnboundVariable)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:1071:53 - error: "before_ts" is possibly unbound (reportPossiblyUnboundVariable)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:1074:12 - error: "before_ts" is possibly unbound (reportPossiblyUnboundVariable)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:1076:30 - error: "before_ts" is possibly unbound (reportPossiblyUnboundVariable)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:1117:23 - error: "before_ts" is possibly unbound (reportPossiblyUnboundVariable)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:1119:53 - error: "before_ts" is possibly unbound (reportPossiblyUnboundVariable)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:1123:34 - error: "before_ts" is possibly unbound (reportPossiblyUnboundVariable)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:1400:16 - error: Operator "not in" not supported for types "Literal['users']" and "Any | None"
    Operator "not in" not supported for types "Literal['users']" and "None" (reportOperatorIssue)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:1403:29 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:1412:20 - error: Operator "not in" not supported for types "Literal['next_token']" and "Any | None"
    Operator "not in" not supported for types "Literal['next_token']" and "None" (reportOperatorIssue)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/api.py:1414:43 - error: Object of type "None" is not subscriptable (reportOptionalSubscript)
/Users/jackson/projects/github.com/JOJ0/synadm/synadm/cli/_helper.py
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/cli/_helper.py:251:31 - error: Cannot access attribute "target" for class "list[Unknown]"
    Attribute "target" is unknown (reportAttributeAccessIssue)
  /Users/jackson/projects/github.com/JOJ0/synadm/synadm/cli/_helper.py:251:49 - error: Cannot access attribute "port" for class "list[Unknown]"
    Attribute "port" is unknown (reportAttributeAccessIssue)
24 errors, 0 warnings, 0 informations

If it mentions None, it's probably not correctly handling the API's None responses correctly. pyright also doesn't detect API usages because of the click helper thing, so this is not complete as it misses CLI stuff.

So some of my ideas with how to proceed with this codebase-wise:

  • Continue as-is, and handle a potential lack of response completely every time (if not obvious, this is not the case even within this codebase)
  • Throw exceptions in the query function in the API, creating a breaking change to handling failed requests (no clear design about this yet)
    • Log fatally, exiting the program early, when querying Synapse
  • Something else

I'm currently not sure. Might think about this later.

(Also please don't cut off stacktraces!)

@JOJ0
Copy link
Owner Author

JOJ0 commented Jan 23, 2025 •
edited
Loading

Log fatally, exiting the program early, when querying Synapse

This seems like the best idea here.

A try/except wrapper around "everything" (not sure yet what everything is) or maybe just around the query function? Would that work?

Another idea would be to more precisely return what Synapse/requests "experiences". Elsewhere (and I know it's a very basic and maybe not the most sophisticated strategy) I had requests functions like query return a 3-valued tuple (respone|None, errcode|None, errdetail|None) (or could also be a named tuple or dataclass). At least this would give an easy opportunity on the CLI level to put simple "early returns" and end the program:

if errcode != 200:  # or check for only "fatal" errorcodes/errortypes
    log.error(errdetail)
    sys.exit()

Again, this is very basic, also it would mean we would have to change every CLI and every API function. But once we had it in place, it might help with "reacting" to other less-fatal errors and work with that. And yes I'm aware of that this is something similar or maybe just a little better as what you point out in "- Continue as-is ....." , but still more control....and we still would have to finally do it everywhere

For fatal-only probably putting a "global" wrapper "somewhere" and simply ending sounds easier to implement.

@JOJ0
Copy link
Owner Author

JOJ0 commented Jan 23, 2025

...and now that you mention typing.....we should talk about that too ;-)

@JOJ0 JOJ0 linked a pull request Jan 24, 2025 that will close this issue
Open
@JacksonChen666 JacksonChen666 linked a pull request Jan 24, 2025 that will close this issue
Exit synadm early on fatal errors, refactor & fix query() #168
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

issue-164 JOJ0/synadm
Exit synadm early on fatal errors, refactor & fix query() JOJ0/synadm
2 participants
@JOJ0 @JacksonChen666