?

Log in

No account? Create an account

Previous Entry | Next Entry

I was forwarded a support request about some iJournal-related problems and it seems you guys are doing some wacky stuff that's causing you problems.


I can't look at the code directly (sourceforge is broken again... does this surprise anyone?) but it seems at least a few of these apply to iJournal:
  • You assume moodids are contiguous.
  • You aren't caching moods.
  • Your protocol response parser is broken in some way.


First, nobody else has reported this problem with moodid 50, and the word from Brad is:
Reviewed the code, seems fine.

...

And there is no 50:

mysql> select * from moods where moodid between 48 and 55;
+--------+---------------+------------+
| moodid | mood          | parentmood |
+--------+---------------+------------+
|     48 | indescribable |          0 |
|     49 | sleepy        |         31 |
|     51 | groggy        |         31 |
|     52 | hyper         |         11 |
|     53 | relaxed       |         15 |
|     54 | restless      |         74 |
|     55 | disappointed  |         25 |
+--------+---------------+------------+
7 rows in set (0.00 sec)   


So they're probably assuming things are contiguous.

Here's a clip from the docs for the login mode:
If your client supports moods, send this [getmoods] key with a value of the highest mood ID you have cached/stored on the user's computer. For example, if you logged in last time with and got mood IDs 1, 2, 4, and 5, then send "5" as the value of "getmoods". The server will return every new mood that has an internal MoodID greater than 5. If you've never downloaded moods before, send "0". If you don't care about getting any moods at all (if your client doesn't support them), then don't send this key at all.

It's not stated explicitly, but that example shows the possibility of missing mood id 3. But really, even if you assumed all the moodids did exist, a well-designed client should notice that moods[50] is null and not break in other ways.

Now, this problem should not really be coming up for most people because you definitely are supposed to cache the mood list client-side. That's why getmoods takes an argument. So once you have the mood list once, the users should never be experiencing any parse errors while parsing a mood list, because they shouldn't be getting a mood list on subsequent logins.

Finally, I'm not sure what your parser is doing, but the absence of a moodid 50 should not corrupt other keys (unless LJ really is outputting invalid responses, but I don't think it is). I won't venture to guess what you're doing, but it really shouldn't be that complicated (otherwise we might as well all be using XML-RPC). Here's a clip from logjam, where res is the server response, util_getline returns an allocated copy of the current line, and util_skipline returns a pointer to the next line:
    while (*res != 0) {
        key = util_getline(res);
        res = util_skipline(res);
        if (*res) {
            val = util_getline(res);
            res = util_skipline(res); 
            /* insert key/val pair into hash table. */
            g_hash_table_insert(hash, key, val);
            read_keys++;
        }
    }


Finally, here's a dump from when I login with the "test" account (after clearing my local mood cache):
mood_48_name
indescribable
mood_49_id
49
mood_49_name
sleepy
mood_4_id
4
mood_4_name
anxious
mood_50_id
51
mood_50_name
groggy
mood_51_id
52
mood_51_name
hyper

This tells me that moodid 49 is sleepy, 4 is anxious, 51 is groggy, and 52 is hyper.

Please let me know if you need any more information!

Comments

( 9 comments — Leave a comment )
naughtypixie
Jan. 16th, 2003 11:30 pm (UTC)
Thanks for the information (i'm not an iJournal Developer at all, but i'm sure they'll be reading this this soon), but is there some reason that this would have suddenly occurred when the last release of iJournal was sometime around August last year, and it's been working absolutely fine up until then?

Pardon if this sounds like an attack, it's not at all and i'm genuinly curious, but it would seem that if it were just an issue with the iJournal code, then it would have appeared shortly after the release rather than happenening to all the users near simultaniously at a point some 5 months afterwards (especially considering how frequently we all use it). The only other thing I can can think of is the change in the year which happened more recently, though it was still working fine for us for nearly 2 weeks before these issues occured.

That being the case, has the been any change at all to the LJ code, one still within the LJ standards but perhaps one that exposed this potential lurking issue for us iJournal users, which may lead to our iJournal developers discovering a fix for the problem quickly?
evan
Jan. 17th, 2003 12:01 am (UTC)
Hey, that's no attack. Now that I read over it again, it's quite likely LJ is doing something wrong.

I think iJournal not caching moods would make it exhibit this problem where no other LJ clients would (because they only retrieve moods once).

If it is on LJ's end, I would be more inclined to guess that the problem is related to load, because even when I tried to login with "test" to produce the dump above I had some malformed responses from LJ. :)

As for potential lurking issues, the only one that's been a problem in the past is client authors misparsing newlines, but I doubt that'd pop up in the middle of the post. If anyone can give me a dump of a malformed response I'd be able to give better information...
cryo
Jan. 17th, 2003 12:12 am (UTC)
Caching wasn't implemented yet (on the list).

I see the problem though... but I'm blamimg LJ.

The mood ID must equal the number of the ID


mood_49_id
49
mood_49_name
sleepy


Correct.


mood_4_id
4
mood_4_name
anxious


Also correct.


mood_50_id
51
mood_50_name
groggy


that's the problem. It's not that the ID's aren't contiguious, but that the mood_XX_id doesn't match its result.

Since this has been done since day 1, and is the basis of how the dictionary is built and the mood array displayed, it can either a) be changed back to what it was doing and not have thousands of people have to download a new client version when we aren't ready to do a release yet...
or a).

If anyvalue_#_id is -not- going to equal the # returned, then there was no point in making a big deal about contiquity when it's basically saying now foo_XX_id -will- be contiguious (which would have eliminated a huge hack I did in the code... and much angst on my part ;) ).

So this kinda sucks, because the current source isn't ready to be released, but is fairly close... and the old client isn't going to work.
cryo
Jan. 17th, 2003 12:14 am (UTC)
This will break the other arrays too, and may explain why friends/groups are having trouble displaying correctly.

cryo
Jan. 17th, 2003 12:35 am (UTC)
Was the only change to remove ID 50? I thought there are other missing IDs... and I did a lot of testing to assure that the right moods were being sent and displayed. Hmm.


Here's what needs to be changed (I think... I haven't tested it yet, and it's way too early in the morning...zzz):

// Setup Moods
moodCount = [myDictionary objectForKey: @"mood_count"];
moodArray = [NSMutableArray arrayWithCapacity:[moodCount intValue]];

for (i = 1; i <= [moodCount intValue]; i++) {

if (myKey = [NSString stringWithFormat: @"mood_%d_name", i]) {

curVal = [myDictionary objectForKey: myKey];
myKey = [NSString stringWithFormat: @"mood_%d_id", i];
myIndex = [myDictionary objectForKey: myKey];
[moodArray insertObject: curVal atIndex: [myKey intValue]];
[mood setObject: curVal forKey: myIndex];
[mood setObject: myIndex forKey: curVal];

}
}
// sort it
moodArray = [[moodArray sortedArrayUsingSelector:@selector(compare:)
] retain];
evan
Jan. 17th, 2003 01:33 pm (UTC)
It's actually been this way for years. AFAIK, we haven't removed a mood in a long time.

It's not that the ID's aren't contiguious, but that the mood_XX_id doesn't match its result.
If it did match, what use would the protocol have sending n mood_xx_id keys, each with the same value xx? We could just send mood_xx_name and be done with it.

You're confusing two ideas:
- the mood ids (which are arbitrary, tend to mostly be near zero, but are not required to be contiguous)
- the indicies into the array of (moodid, moodname) returned by the protocol: these must be contiguous. For example, based on your bit above, array[50] = (51, "groggy").
The fact that these numbers often line up tends to be confusing...

If your client doesn't handle this properly then it was broken in the first place.


As for missing a field in the response, I'm inclined to think it's either a server issue or your client is mishandling something (did you write your own HTTP routines? the response to the login mode is longer than in other modes, so it's possible you're mishandling multiple subsequent calls to read()).

The LJ code is pretty straightforward:
        my $mood_count = 0;
        foreach my $m (@{$rs->{'moods'}}) {
            $mood_count++;
            $res->{"mood_${mood_count}_id"} = $m->{'id'};
            $res->{"mood_${mood_count}_name"} = $m->{'name'};
        }
        if ($mood_count) {
            $res->{"mood_count"} = $mood_count;
        }


And again, I stress this is not a change; according to CVS logs, some similar-looking code been around since the beginning of CVS history (mid-2001).
wooble
Jan. 17th, 2003 04:58 am (UTC)
Oops. I was actually referring to the "mood_50_name" coming up blank, not realizing that "mood_50_id" was set to 51. It's actually irrelevant, although the problem is different than what I stated.

sometimes the server sends:
mood_4_id
4
mood_4_name
anxious
mood_50_id
51
mood_50_name
groggy
mood_51_id
52

and sometimes it sends:
mood_4_id
4
mood_4_name
anxious
mood_50_id
51
mood_50_name

mood_51_id
52

with just a blank line for the name of mood 51. It seems to happen completely at random.
cryo
Jan. 17th, 2003 09:46 am (UTC)
a blank would be bad, too. The array gets indexed by both the ID and the 'mood', so it would also be unhappy with no mood in a valid ID (or rather, invalid ID).
wooble
Jan. 17th, 2003 10:12 am (UTC)
Yes, a blank is bad. It shows that the problem isn't really with iJournal (although if we were caching moods it wouldn't come up, assuming the user's cache had been built before the server started acting flaky; new users might get stuck with completely broken moods forever if we didn't know to check for this bug), and it has nothing to do with assuming that mood IDs are contiguous (we don't).
( 9 comments — Leave a comment )

Profile

ijournal
iJournal: Official LiveJournal Client for Mac OS X
iJournal Home

Latest Month

January 2012
S M T W T F S
1234567
891011121314
15161718192021
22232425262728
293031    
Powered by LiveJournal.com
Designed by Lilia Ahner