Fix webhook listener stalling (hopefully) - #144
Conversation
The most notable thing here is removing a `sys.version_info.major` check that we no longer need (Sopel 8 already requires Python >=3.8, and the plugin's requirements now specify sopel>=8). It must have been a while since anyone opened this in an IDE with import linting, though. All I did was look for dimmed/grayed-out imports after opening the project in VS Code today.
On a whim, I decided to see if VS Code's built-in copilot chat could help figure out our intermittent issue with the webhook server sometimes becoming unresponsive. I briefly summarized the problem and gave it a direction to investigate (`wsgiref` and its basic server implementation). The copilot chat chose Claude Sonnet 5 to help with this, and initially it spat out a whole litany of changes to this section of the code adding multi-threading support to the server on top of the timeout. I asked it to scale back and just try the timeout for now, "one thing at a time", then tweaked the output and fenced it with the comments marking exactly what section I let the matrix-multiplication bot touch. The disclaimers are included out of a perhaps-unreasonable desire for transparency. I followed the imports myself to verify that there is indeed a `timeout` to set somewhere up the tree, but I feel it's fair to explicitly mark that this approach originated from an LLM.
Python 3.10 deprecated this method of setting a Thread object's daemon status. It is allowed to set the `Thread.daemon` property directly before the Thread has been started, but since there's also a `daemon` kwarg available (in Python 3.8+; I didn't go back further than Sopel 8's supported range) let's just use that when creating the Thread.
SnoopJ
left a comment
SnoopJ
left a comment
There was a problem hiding this comment.
Change itself looks fine, I mostly had thoughts about how provenance is being indicated.
| # BEGIN Claude-assisted tweaks (Sonnet 5, VS Code auto-selected) | ||
| # wsgiref never times out reads, so a stalled/half-open client | ||
| # connection can hang the server indefinitely without this. |
There was a problem hiding this comment.
I have mixed feelings about the way provenance is being indicated here. On the one hand, it's laudable to track where the code came from (or rather, the idea for the change, since the actual amount of code here is marginal). On the other hand, half of this commit is that kind of commentary!
I think I would be happier without the BEGIN/END, but rather noting that an LLM assisted and pointing to this PR in the comment right by the timeout.
Not a strong opinion, but I thought I'd share how I feel about the practice given here.
There was a problem hiding this comment.
The idea was to fence off the area Claude touched, including its tweaks to how we set the request handler class. You really think it's too much?
There was a problem hiding this comment.
Yea it feels like duplicating the information in the commit itself a little bit, especially when I consider what such a thing would look like over time if LLM tools were touching the same bit of code, and you ended up with overlaps (which seems likely, since they help create mistakes all the time).
Not a strong set of opinions, but maybe I'm underestimating how much of the lifting the tool did here.
There was a problem hiding this comment.
My edits were pretty minor, so I didn't want to claim credit for authoring the changed lines. I guess you could argue that once I double-checked Claude's work against docs and stdlib source code, and decided that even without the tool I would've arrived at ~the same patch, the fencing isn't really relevant any more?
For this patch I obviously skewed really heavily toward making it obvious which part of the code was touched by matrix multiplication, without even having to look at the commit/blame log. That works for small, isolated tests of those tools such as this. Worth rethinking if any of us keep using them on this codebase, but I suspect that won't happen. After another, green-field, trial drafting something else, I have decided that the best use of those tools is to have them suggest something, then read it for inspiration before rewriting from scratch. Then the tool helps me learn about interfaces I wasn't familiar with, and I still fully understand the end product.
(Going a wee bit off topic, aren't I? But I still think it's worth saying, so I'll leave it be.)
| handler_class = self.options.pop('handler_class', WSGIRequestHandler) | ||
| if self.quiet: | ||
| class QuietHandler(WSGIRequestHandler): | ||
| class QuietHandler(handler_class): | ||
| def log_request(*args, **kw): | ||
| pass | ||
| self.options['handler_class'] = QuietHandler | ||
| handler_class = QuietHandler |
There was a problem hiding this comment.
The multiple assignment here makes me raise my eyebrows a little bit, but since it's gated behind a flag where we add a wrapper, I think it's okay to leave it as is.
There was a problem hiding this comment.
What pattern would you use if not this?
I'm in favor of the change to make specifying the handler class more explicit even on the default path (when not self.quiet).
There was a problem hiding this comment.
If I were going to change it to avoid the multiple assignment, I'd probably name the first assignment base_handler_class, then have a trivial assignment (handler_class = base_handler_class) in the non-subclass case.
Alternatively, one might duplicate the assignment to self.options instead (i.e. self.options['handler_class'] = QuietHandler on one branch and the base-case on the other).
But I think as you've written it, it's clear enough. This is a case where multiple assignment just kinda is the best way to write it, especially since we pop()ed the original.
| # Claude defaulted to 60 seconds; dgw thinks 10 is more reasonable for | ||
| # our webhook listener since GitHub itself will give up pretty quickly. | ||
| handler_class.timeout = 10 |
There was a problem hiding this comment.
I suppose it could be a configurable option, but that's probably overthinking things. What you have looks fine to me.
|
Have deployed this to a test branch on the Sopel instance serving our channels on Libera, as a copy of |
2 participants
I wanted to see if we can avoid, or at least greatly reduce, instances of our Libera bot's webhook listener becoming unresponsive. This is the result of asking VS Code a question, telling it not to change so much at once, and researching the eventual patch myself before committing it. A second look from another human would be awesome, before testing this "in production" on our Libera bot for a while (weeks or months; it's an infrequent and intermittent issue) before considering a merge to the main branch.
The end result is still disclosed as Claude Sonnet 5–assisted in my final diff, because I believe in FULL transparency regarding when "AI" was used.
The "real" patch is sandwiched by housekeeping changes. I'm not sure whether there's such a thing as a "webhook sandwich", but maybe I just invented it.