 Notes |
(0014696)
Shalin Singh (reporter)
2010-01-07 09:17
|
Patch contributed by Dahlia Trimble (attached) solves the problem.
It might also be noticed that this patch prevents another problem: a message "User not logged in, message saved." was sent back to the group UUID (but as a non-group message). I suppose it was discarded then.
So this patch will increase performance in two ways:
- not storing group IMs into the database
- not sending useless confirmations
Tested with success on New World Grid. |
|
(0014697)
Shalin Singh (reporter)
2010-01-07 09:22
|
Patch by Dahlia Trimble solves the issue, included. |
|
(0014698)
Fly-Man- (developer)
2010-01-07 10:15
|
Uhm, maybe I am reading this wrong, but Group Notices are being saved to the database. Have never seen group im's even being saved in the database. |
|
(0014699)
Shalin Singh (reporter)
2010-01-07 10:35
|
Vinc from Francogrid says the same as you: no group IM is stored in the database.
But we have just sent a group IM in some arbitrary group (with the normal client), and the patch catches it, you can see it in the region log.
The only rationale explanation to this apparent contradiction is that this piece of code is not the place where messages to offline people are handled to the database.
But then, why are people not spammed anymore by group messages when they log again, when the patch is applied ? After all, it works. |
|
(0014700)
Shalin Singh (reporter)
2010-01-07 10:40
|
Someone just suggested to me an hypothesis that might explain everything.
Perharps the mechanism to store offline ims is not via the database?
And the database cluttering would be another, independant, problem? |
|
(0014701)
mcortez (developer)
2010-01-07 10:41
edited on: 2010-01-07 10:41
|
Technically groups aren't storing the IMs, it's the InstantMessage & OfflineMessageModule that's storing them.
The patch looks good to me and if it hasn't been applied, I'd suggest that it be applied. My only recommendation would be to possibly make it configurable, but that may be more effort then it's worth.
|
|
(0014702)
Fly-Man- (developer)
2010-01-07 10:41
|
+1, reviewed the patch and it seems that the group im's uses the same logic as for the normal avatar offline im's thus making it save everything for a user that's not online.
Although, the debug it sends to the console is not needed. When you have more then 10 groups on your grid, it will flood the console ... |
|
(0014703)
nebadon (developer)
2010-01-07 10:43
|
group IMs are translated into standard IMs i believe Fly Man. then if offline messages is enabled they will be stored. |
|
(0014704)
Shalin Singh (reporter)
2010-01-07 10:50
|
Yup, the offline messages are stored for later delivery through a REST URL, not via the database. I just had a look at the source code.
I think The problem description is bad. Sorry.
The problem description should simply be "Group IMs should not be kept for later delivery like private IMs".
I guess that the attached patch solves that pecular problem, not the one that is described in this mantis issue. |
|
(0014705)
Shalin Singh (reporter)
2010-01-07 10:52
|
Yes, the debug output was just here to prove that the patch was working.
+1 to nuke it. |
|
(0014706)
mcortez (developer)
2010-01-07 10:54
|
If someone got bored, I'd also suggest making Offline messages have a delivery cap, or throttle, so even *if* you have too many offline messages stored up, they don't overload a standard SL based client on login. |
|
(0014709)
Shalin Singh (reporter)
2010-01-07 11:12
|
@mcortez : yes.
The probability to get flooded by *group* IMs when you are away is way higher than the probability to get flooded by *personal* IMs when you are away. But that does not mean that the second probability is non-existent :-(. |
|
(0014711)
dahlia (manager)
2010-01-07 11:35
|
commit 1e899704c1c19a8c42ff313677a13f35b46605da adds a new config option "ForwardOfflineGroupMessages" which addresses this issue. Please try it out and provide feedback. |
|
(0014713)
Shalin Singh (reporter)
2010-01-07 12:05
|
@Dahlia: I hope that the acknoledgement "System: User not online. Message saved." is not sent back to the group when the group IM is forwarded, due to that config option. This acknoledgement makes no sense for group IMs.
Just my 2 cents. |
|
(0014732)
dahlia (manager)
2010-01-08 16:00
|
@Shalin, I don't believe it is. If you find that it does in your testing, please let me know. |
|
(0014739)
Shalin Singh (reporter)
2010-01-09 00:51
|
Hum, right, it looks like there's a test that prevents it. |
|
(0014740)
Shalin Singh (reporter)
2010-01-09 01:04
|
By the way, that's perharps a style/code readability issue.
Your patch uses the test
if (im.fromGroup)
The existing code uses the test
if (im.dialog == (byte)InstantMessageDialog.MessageFromAgent)
If they are functionally exivalent, then perharps it would be better if they were written the same.
Furthermore, the program flow could be simplified. Current form :
if (offline && (!fromGroup || (fromGroup && forward))
{
store offline message;
if (!fromGroup)
{
send acknowledgement;
}
}
Suggested form :
if (offline)
{
if (!fromGroup || (fromGroup && forward))
{
store offline message;
}
if (!fromGroup)
{
send acknowledgement;
}
}
Looks more logical, no ? |
|
(0014742)
Shalin Singh (reporter)
2010-01-09 04:01
|
Hmmm, no, both tests are not equivalent.
But still, a code reorganization would be beneficial. Here is what I propose:
if (offline)
{
if (!fromGroup || (fromGroup && forward))
{
store offline message;
}
if (dialog == MessageFromAgent)
{
send acknowledgement;
}
}
i.e. move your test to control only the storing of the offline message, not the acknowledgement. |
|
(0014901)
justincc (manager)
2010-01-29 15:38
|
Shalin, if you could stick your code reorg in a patch I'm sure we could look to apply it. |
|
offline-group-ims.patch [^] (664 bytes) 2010-01-07 09:13 
