fix problems with lazy loading of users (etc) - #292
Merged
Conversation
The "silly fallback" in ->username only fired when we had no users hash
at all, which is nearly never. The common case is a users hash that was
loaded before the user in question existed: then we'd return undef, and
whoever called us would warn about an uninitialized value. These two,
for example:
Use of uninitialized value in string eq at Synergy/Channel/Slack.pm line 173.
Use of uninitialized value in concatenation (.) at Synergy/Channel/Slack.pm line 262.
Checking the entry rather than the whole hash also stops us from
autovivifying an empty, nameless user into the cache on every miss.
While we're here, route the two ->users->{$id}{name} lookups in
Channel::Slack through ->username, so they get the same treatment
instead of dying on an undef hashref before the users load.
Co-Authored-By: Claude <noreply@anthropic.com>
In 0be3774, the load_X methods became async and grew an early return when their data was already loaded, so that ->readiness would be cheap to call more than once. That also, quietly, turned every *reload* call site into a no-op: * "reload slack users" reported success and did nothing at all * user_status_for stopped picking up fresh Slack statuses * group_conversation_name stopped noticing new group chats Since then, the only way to learn about a user who joined the workspace after we started up has been to restart Synergy, which is how we end up warning about undefined usernames for days. Now load_X means "make sure we have X" and reload_X means "go get X again, right now", and the call sites above use the latter. The reload command also waits for the reload to finish before saying it's done, rather than dropping the future on the floor. Co-Authored-By: Claude <noreply@anthropic.com>
load_users did not look at the HTTP status or at Slack's own "ok" field.
If users.list said no -- rate limiting, a token problem, a bad gateway
-- then $res->{members} was undef, %users was empty, and we cached an
empty hashref, logged "Slack users loaded", and then failed to name a
single user for the rest of the process's life. Because the cache was
now "loaded", nothing would ever try again.
Everything that populates a cache now goes through ->_api_data, which
fails the future unless Slack really did say yes, so a bad response
leaves the previous cache in place instead of replacing it with
nonsense. An empty user list is treated as a failure too, since we are
ourselves a member of the workspace.
Also, "my $me = $users{...}; $me->{name} = ..." silently autovivified a
throwaway hashref if we weren't in the list we just fetched, so our own
name went uncoerced and nobody heard about it. Now we log it.
The two callers that reload from synchronous code degrade to the cache
they already have, rather than throwing, if the reload fails.
Co-Authored-By: Claude <noreply@anthropic.com>
users.list, like every "list" method in the Slack API, is paginated: it
hands you a page and a cursor, and if you ignore the cursor it looks
exactly like a complete answer. We ignored the cursor everywhere. Once
the workspace outgrew one page, the members on the pages we never asked
for became people we could never name, no matter how often we reloaded.
conversations.list was worse off, since it was asked for a hard limit of
200 public channels and then had its cursor ignored too.
Now all four loads go through ->_api_data_pages, which follows the
cursor (up to a sanity limit of 20 pages) and dies rather than quietly
returning nothing if the response doesn't have the key we asked for.
It also sends these calls form-encoded, which is what the "list" methods
actually want; users.list was the only one being sent as JSON.
While here: conversations.list returns its results in "channels", even
for types=im, so reload_dm_channels was reading a key ("ims", from the
long-deprecated im.list) that isn't there.
Co-Authored-By: Claude <noreply@anthropic.com>
The dm_channels attribute has a non-lazy default of {}, so it's built
during construction, so its predicate has been true since before we ever
connected, so load_dm_channels has returned early every single time it
was called. It has never once run. (This is also why nobody noticed it
was reading the wrong key out of the response.)
That's fine, as it turns out: dm_channel_for_address opens DM channels
one at a time and remembers them, which is how every DM channel we've
ever used got found. So rather than add a startup API call we've never
needed -- and a new way for startup to fail -- drop the dead
load_dm_channels, take dm_channels out of ->readiness, and say in a
comment how this cache really works.
reload_dm_channels stays: "reload slack users" uses it to throw away
whatever we've accumulated and start over.
Co-Authored-By: Claude <noreply@anthropic.com>
Two ways, now, that a user id we've never seen can become a name without waiting for a restart: First, we pay attention to the team_join and user_change events on the RTM connection, which is Slack telling us exactly what we want to know. Second, when ->username is asked about an id that isn't in the cache, it asks users.info about that id in the background and answers correctly the next time. It can't wait for the answer, because it's called from places like the s///e that rewrites <@u123abc> in message text, so this time around the caller still gets "<unknown user U123ABC>". We ask only once per id: many of the ids we see -- apps, and people from other workspaces we share channels with -- are not users we will ever be able to look up, and there's no sense asking about them over and over. Also, ->username no longer uses an undefined id as a hash key, which warned in its own right. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.