Closed Bug 954890 Opened 8 years ago Closed 8 years ago

Make use of all WHOWAS 312/314 response pairs

Categories

(Chat Core :: IRC, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: clokep)

References

Details

Attachments

(1 file, 3 obsolete files)

*** Original post on bio 1457 at 2012-05-22 00:18:00 UTC ***

Some servers (eg mozilla) send multiple responses that seem to be in chronological order, giving what amounts to a recent login history. We currently ignore all but the last of these (i.e. the corresponding whois entries are overwritten).
Duplicate of this bug: 954926
*** Original post on bio 1457 at 2012-06-23 13:37:51 UTC ***

(In reply to bug 954926 (bio 1493), comment #0)
> WHOWAS actually returns a sequence of 312/314 messages, e.g. 
> 
> irc: :gravel.mozilla.org 314 adev flo Instantbir moz-87C33FDA.com * :Florian
> irc: :gravel.mozilla.org 312 adev flo gravel.mozilla.org :Thu Jun  7 14:50:13
> 2012
> irc: :gravel.mozilla.org 314 adev flo florian moz-87C33FDA.com * :purple
> irc: :gravel.mozilla.org 312 adev flo gravel.mozilla.org :Thu Jun  7 12:54:30
> 2012
> irc: :gravel.mozilla.org 314 adev flo Instantbir moz-87C33FDA.com * :Florian
> irc: :gravel.mozilla.org 312 adev flo concrete.mozilla.org :Thu Jun  7 12:02:18
> 2012
> irc: :gravel.mozilla.org 369 adev flo :End of WHOWAS
> 
> Currently we only use the final pair (as the previous entries in
> whoisinformation get overwritten). This is less informative and actually
> incorrect, as they are sorted in descending time order.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1457 as attmnt 1898 at 2012-09-04 03:14:00 UTC ***

This uses the first response we get from WHOWAS, I might have made this a bit more complicated than necessary (perhaps we just shouldn't overwrite the value if it exists already?), but it seemed reasonable.

I also debated trying to parse the date, but:
1. Not all servers give it.
2. Parsing dates is a PITA.
Attachment #8353656 - Flags: review?(bugzilla)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment on attachment 8353656 [details] [diff] [review]
Patch

*** Original change on bio 1457 attmnt 1898 at 2012-09-04 10:50:18 UTC ***

This seems to fail to properly handle the corresponding WHOIS responses.
Attachment #8353656 - Flags: review?(bugzilla)
*** Original post on bio 1457 as attmnt 1900 at 2012-09-05 01:09:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353658 - Flags: review?(bugzilla)
Comment on attachment 8353656 [details] [diff] [review]
Patch

*** Original change on bio 1457 attmnt 1898 at 2012-09-05 01:09:02 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353656 - Attachment is obsolete: true
*** Original post on bio 1457 at 2012-09-05 21:24:09 UTC ***

This works, but I am wondering (as you indicate in comment 3) why we are going to be storing all the responses for 312 and not the paired ones for 314 (so the current patch prints the most recent 312 with the oldest 314), and if it's just the mozilla server where the 312/314 pairs arrive in reverse chronological order (so that the assumption this patch is based on is more valid then the existing "use the last arrival" code).
*** Original post on bio 1457 at 2012-09-06 00:49:40 UTC ***

(In reply to comment #6)
> This works, but I am wondering (as you indicate in comment 3) why we are going
> to be storing all the responses for 312 and not the paired ones for 314
I don't follow this at all, aren't we storing all the 312 responses and 314 responses?

> (so the current patch prints the most recent 312 with the oldest 314),
How so? Don't we use the newest one (which is the first received response) for both? I'm not sure how you know what is "newest" for 314, I'm just assuming it goes with the closest 312.

> and if it's just
> the mozilla server where the 312/314 pairs arrive in reverse chronological
> order (so that the assumption this patch is based on is more valid then the
> existing "use the last arrival" code).
I'm not really sure what this is talking about...
Comment on attachment 8353658 [details] [diff] [review]
Patch v2 - store all received entries

*** Original change on bio 1457 attmnt 1900 at 2012-09-06 10:56:19 UTC ***

(In reply to comment #7)
> > to be storing all the responses for 312 and not the paired ones for 314
> I don't follow this at all, aren't we storing all the 312 responses and 314
> responses?
Uh, right, sorry. I got confused by looking at the diff on bugzilla only.

> > and if it's just
> > the mozilla server where the 312/314 pairs arrive in reverse chronological
> > order (so that the assumption this patch is based on is more valid then the
> > existing "use the last arrival" code).
> I'm not really sure what this is talking about...
The net effect of the patch is that we show the first pair to arrive rather than the last. I was just wondering if we know that this is on average more likely to be the right one. But so far the only server I've seen that even returns more than one pair is mozilla.org so I guess the answer is "yes" ;)
Attachment #8353658 - Flags: review?(bugzilla) → review+
*** Original post on bio 1457 at 2012-10-12 10:42:37 UTC ***

(In reply to comment #8)
> Comment on attachment 8353658 [details] [diff] [review] (bio-attmnt 1900) [details]
> Patch v2
>
> > > and if it's just
> > > the mozilla server where the 312/314 pairs arrive in reverse chronological
> > > order (so that the assumption this patch is based on is more valid then the
> > > existing "use the last arrival" code).
> > I'm not really sure what this is talking about...
> The net effect of the patch is that we show the first pair to arrive rather
> than the last. I was just wondering if we know that this is on average more
> likely to be the right one. But so far the only server I've seen that even
> returns more than one pair is mozilla.org so I guess the answer is "yes" ;)

So do we think this is fine the way it is...or should we stop storing all the pairs and simply use the last one. But if so, isn't that the same as what we previously had?
*** Original post on bio 1457 at 2012-10-12 16:43:18 UTC ***

(In reply to comment #9)
We should either take this patch as is or simplify it to just show the first pair (correct on mozilla.org, so far our only use case). Taking it as is might make it easier to make changes later if other server behaviours turn up (or to show all the received info if anyone cares).
Attached patch Alternate patch v1 (obsolete) — Splinter Review
*** Original post on bio 1457 as attmnt 1955 at 2012-10-12 18:28:00 UTC ***

This will only use the first whois set if one is repeated.
Attachment #8353714 - Flags: review?(bugzilla)
*** Original post on bio 1457 as attmnt 1956 at 2012-10-12 19:04:00 UTC ***

I received some feedback over IRC.
Attachment #8353715 - Flags: review?(florian)
Comment on attachment 8353714 [details] [diff] [review]
Alternate patch v1

*** Original change on bio 1457 attmnt 1955 at 2012-10-12 19:04:54 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353714 - Attachment is obsolete: true
Attachment #8353714 - Flags: review?(bugzilla)
Comment on attachment 8353715 [details] [diff] [review]
Alternate patch v2

*** Original change on bio 1457 attmnt 1956 at 2012-10-16 19:10:24 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353715 - Flags: review?(florian) → review?(bugzilla)
Comment on attachment 8353715 [details] [diff] [review]
Alternate patch v2

*** Original change on bio 1457 attmnt 1956 at 2012-10-16 19:15:16 UTC ***

as seen on IRC ;)
Attachment #8353715 - Flags: review?(bugzilla) → review+
Comment on attachment 8353658 [details] [diff] [review]
Patch v2 - store all received entries

*** Original change on bio 1457 attmnt 1900 at 2012-10-16 19:17:12 UTC ***

Tagging this in case it is ever needed again in another context.
Attachment #8353658 - Attachment description: Patch v2 → Patch v2 - store all received entries
Attachment #8353658 - Attachment is obsolete: true
Whiteboard: [checkin-needed]
*** Original post on bio 1457 at 2012-10-25 12:06:31 UTC ***

(In reply to comment #12)
> Created attachment 8353715 [details] [diff] [review] (bio-attmnt 1956) [details]
> Alternate patch v2

Committed as http://hg.instantbird.org/instantbird/rev/d30db418fd7c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.3
You need to log in before you can comment on or make changes to this bug.