coretasks: Periodically send WHO to keep user information up-to-date

[rdswift]

Add an interval event to periodically issue a WHO request to keep user information up-to-date.

This provides part of the solution to Issue #1659 : User's "away" status always "None"

[dgw]

Not a bad idea. We should definitely put some thought into the interval duration, based on how other bots and clients behave. More importantly, Sopel shouldn't do any WHO polling for away statuses when the away-notify capability is enabled, because AWAY messages from the server will take care of it:

sopel/sopel/coretasks.py

Lines 822 to 831 in 9340c67

	@sopel.module.rule('.*')
	@sopel.module.event('AWAY')
	@sopel.module.priority('high')
	@sopel.module.thread(False)
	@sopel.module.unblockable
	def track_notify(bot, trigger):
	if trigger.nick not in bot.users:
	bot.users[trigger.nick] = User(trigger.nick, trigger.user, trigger.host)
	user = bot.users[trigger.nick]
	user.away = bool(trigger.args)

Our JOIN handling already takes care of sending a WHO on joining a new channel, fortunately. On networks that support away-notify (something like 84% of them, at last survey), #1663 alone will fix away-status tracking. This is only needed for the ~16% of networks that don't support the newer method yet.

[rdswift]

I agree. This PR should be updated to use a reasonable interval, and to avoid sending the WHO request if the away-notify capability is enabled. I assume there's an easy way to test for that?

[dgw]

'away-notify' in bot.enabled_capabilities or something like that should do. Simplest way is to just return at the beginning of this added function if that's true. Big overhaul of network properties coming with #1536 (hopefully), which might change it, but that test should work right now.

As far as the interval duration itself, I've done a small survey of clients I use or have used in the past:

• Textual: 120 seconds
• HexChat: update 1 channel every 30 seconds
• KVIrc: update oldest WHO list if older than 150 seconds
• The Lounge: appears to do no polling at present, relying on away-notify only

HexChat's is probably the nicest approach, but also the most complex to implement and the most likely to have outdated information (since as the bot's channel count grows, it takes longer between updates of a given channel's status information). I'd say KVIrc's approach is the next-nicest.

[rdswift]

The KiwiIRC client looks to be the same as HexChat (which is what I based my 30 seconds on). I agree the KvIRC approach seems to make the most sense. I'll try to start taking a look at this tomorrow.

[rdswift]

Further to the discussion, I've included some additional logic around sending periodic WHO requests. It is sort of a hybrid between the HexChat and KvIRC approaches.

This function checks every 30 seconds to see if any channel's last WHO request was greater than 120 seconds ago, and initiates a request for the channel with the oldest previous request time. It does not issue more than one WHO request every 30 seconds, and does not issue more than one WHO request for a channel within 120 seconds.

The Channel class in target.py was modified to provide a class attribute to track the last time a WHO request was issued for the channel.

I have no idea why this would be causing one of the DuckDuckGo search engine tests to be failing under Python 2.7. I suspect that might be an unrelated issue?

[Exirel]

I like the interval approach, that's the right tool for the job. So, good work on that!

On the other hand, I feel like the code is a bit complicated to me, and I got confused with the timestamp manipulation. Using an integer can be a good idea, but it's not immediately clear why the magic number 1. I'd rather see None keeping its semantic value of "not set yet" and datetime objects compared to each other, like this:

elapsed_time = datetime.today() - timedelta(seconds=120)
if channel.last_who is None or channel.last_who <= elapsed_time:
    # request a who

Other small nitpicks are the usage of 1 as a default value while None has a useful semantic value that is lost with that 1, making thing a bit confusing to me; and the very long name and the very long line that comes with it - nothing that can't be fixed.

[rdswift]

I tightened up (and simplified) the code a bit and got rid of the "arbitrary" comparison value. Also changed some of the variable names to more accurately reflect their purpose (and shorten them a bit). I think the logic is a bit clearer now. Is this more in line with what you were looking for?

[dgw]

Aside from line-length (even I, usually laissez-faire about long lines, think these ifs are too long), I think the loop through all channels can be broken early if it's obvious that the bot has found a candidate that will not be replaced (e.g. upon finding a channel with last_who == None).

It might also be easier to read the logic if you don't shy away from nesting if/elif/else blocks. Often, that's clearer than this sort of and/or chaining with parenthesis groupings.

I have no idea why this would be causing one of the DuckDuckGo search engine tests to be failing under Python 2.7. I suspect that might be an unrelated issue?

It just happens sometimes for certain tests, usually from search.py. We're still figuring out the best way to ignore or retry transient errors like that on Travis, since they do affect PR mergeability. Me kicking the failed test run manually usually fixes it.

[rdswift]

@dgw,

I've added the break from the loop as you suggested. Good catch! I've also broken down each test separately which should make the logic quite clear. There are a couple of lines that could be combined to avoid some duplicate code, as:

    who_trigger_interval = 120
    who_trigger_time = datetime.datetime.utcnow() - datetime.timedelta(seconds=who_trigger_interval)
    selected_channel = None
    for channel in bot.channels:
        if channel.last_who is None:
            selected_channel = channel
            break
        if selected_channel is None or channel.last_who < selected_channel.last_who:
            selected_channel = channel
    if (selected_channel is not None):
        if selected_channel.last_who is None or selected_channel.last_who < who_trigger_time:
            _send_who(bot, selected_channel)

I think the logic is still clear, but I'm biased because I developed it. 😉 I'm okay with it the way it is now (each test separate), or I can make these changes if you prefer.

[Exirel]

Ah! I like where you are going with that. Let me push that even further:

    who_trigger_interval = datetime.timedelta(seconds=120)
    who_trigger_time = datetime.datetime.utcnow() - who_trigger_interval
    selected_channel = None

    for channel in bot.channels:
        if channel.last_who is None:
            # WHO was never sent yet to this channel: stop here
            selected_channel = channel
            break
        if channel.last_who < who_trigger_time:
            # this channel's last who request is the most outdated one at the moment
            selected_channel = channel
            who_trigger_time = channel.last_who

     if selected_channel is not None:
             # selected_channel's last who is either none or the oldest valid
             _send_who(bot, selected_channel)

The selected_channel will always be None if there is no channel that match the condition: who is None, or there is an oldest one. The trick is to initialize the who_trigger_time to the base limit, then to update that limit whenever we found a more outdated channel.

[rdswift]

@Exirel, that suggestion is brilliant! I'll update the PR accordingly.

[Exirel]

A tiny nitpick (if-conditions usually don't need parenthesis), and we are all good to go. This is a great feature, and I'm happy to see it ready so fast.

[dgw]

Great iteration, everyone!

I usually prefer to squash the code-review stuff out of PRs before merging. But this time, the evolution of the feature is so clean compared to how it usually happens ("fix:" and "Update filename.py" commits scattered around) that I don't think we should bother squashing. @Exirel, agree?

[Exirel]

Squash it or not, it's a 👍 🚢