0001200: "randomguy" causes clients to freeze in online games

(0005465)
Dusk (developer)
2012-12-03 14:10

This is the see state:

  See:
    TROO A 0 A_Jump(256,"vile","head","skel","boss","imp","spid","fat")
    goto see

The client does not perform the jump until the server instructs it to, so it keeps looping. The stack is overflown before the client gets the order to jump.

(0005466)
ZzZombo (reporter)
2012-12-03 15:04

Maybe use "wait" instead of "goto See"?

(0005467)
Zalewa (developer)
2012-12-03 16:29

ZzZombo: In my opinion if something works correctly offline then it should also work correctly online. No excuses about "WAD being bad".

(0005468)
Dusk (developer)
2012-12-03 16:43

It's a crash and should be addressed...

(0005469)
ZzZombo (reporter)
2012-12-03 16:59

I just provide a possible workaround to the problem that can bu used unless you will make a patch, that's the point.

(0005481)
Dark-Assassin (administrator)
2012-12-07 21:31

For online use, at least 1 tic is needed for A_Jump loops.
This code would have to be modified to compensate.

(0005759)
Torr Samaho (administrator)
2013-01-13 10:47

As Dusk pointed out, the see state is not compatible with Zandronum's jump handling and I don't see any proper way to fix this without completely throwing away Carn's jump handling.

As workaround we could simply try to break infinite loops on the clients (always risking to break valid but very long state combinations of zero duration).

(0005760)
Zalewa (developer)
2013-01-13 11:00
edited on: 2013-01-13 11:07

Quote from "Torr Samaho"
As workaround we could simply try to break infinite loops on the clients (always risking to break valid but very long state combinations of zero duration).

This looks reasonable enough. After all, the clients should synchronize with server eventually (I hope they will) so the player would only see a desynch comparable to what happens with Lost Souls now. This is definitely better than a crash that freezes some computers so strongly that the only option left is to press the reset button. Furthermore a warning could be printed to console when this happens, preferably only once per single game/map, so the modder knows that something is wrong with his decorate.

(0005761)
Torr Samaho (administrator)
2013-01-13 11:18
edited on: 2013-01-13 11:19

Since anything is better than a crash, clients now try to break infinite state loops of zero duration by not allowing any actor to go through more than 10000 states in one tic. This should fix the problems with randomguy. Since this is a very ugly workaround, a warning is printed every time the workaround kicks in.

EDIT: Posted this without seeing your latest response. Printing the warning only once could be a good compromise.

(0005762)
Zalewa (developer)
2013-01-13 11:28

I've tested your build and it appears to work correctly now.

(0005763)
Torr Samaho (administrator)
2013-01-13 11:31

Thanks for checking!

The client side infinite loop workaround warning is now only printed once on each Zandronum instance. Would be nice if somebody confirms this change in the upcoming official 1.1 beta build.

(0005795)
Blzut3 (administrator)
2013-01-17 01:01
edited on: 2013-01-17 01:06

One thing to consider is it's probably fairly common to use A_Jump(256, ...) for jumping to some random state. Any states following these calls will never be run, so perhaps it might be wise to change the client version of A_Jump to set the state duration to -1 if maxchance >= 256 so it waits for the server information?

Edit: While on the topic of knowns in A_Jump. A_Jump(256, "SomeState") is sometimes used instead of 'goto SomeState' since it is resolved at runtime instead of "compile time." This would be another fairly simple condition to check if(maxchance >= 256 && count == 1) and the client would accurately predict this case.

(0005796)
Dusk (developer)
2013-01-17 01:11

I like that, though I think that should be taken to a new ticket.

(0005825)
Zalewa (developer)
2013-01-26 13:51

Tested with build "1.1-alpha-r130120-0905".

Two things I've noticed:
1. Reloading map with 'map' command doesn't reset the 'message was displayed' flag. In other words: to see the message again you have to close the process of the server and start a new one. This might be the intended behaviour, although.

2. When there are multiple offending actor types the message is displayed only for the first offender. Subsequent errors, even for other actors, are ignored silently. As an example I copied entire "RandomGuy" code and pasted it into "RandomGuy2". Summoning RandomGuy displays the message, but if RandomGuy2 is summoned next then the message is not displayed. Reverse summoning order yields the same result. I'm attaching WAD with these actors.

(0005826)
Torr Samaho (administrator)
2013-01-26 19:56

That's the behavior I intended and what I meant with "is only printed once on each Zandronum instance" in the log. Printing it once per instance and per actor type would probably be nicer, but also is more complicated to implement. I decided to go with the simple implementation to have more time to work on other issues.

(0005827)
Zalewa (developer)
2013-01-26 20:15
edited on: 2013-01-26 20:16

Quote from "Torr Samaho"
That's the behavior I intended and what I meant with "is only printed once on each Zandronum instance" in the log.

Roger that.

Quote from "Torr Samaho"
Printing it once per instance and per actor type would probably be nicer, but also is more complicated to implement. I decided to go with the simple implementation to have more time to work on other issues.

I disagree on the complicated part.

std::set<std::string> actors;
if (actors.find(actorType) == actors.end())
{
actors.insert(actorType);
printWarning();
}

I don't consider this fixed until it treats each actor type individually.

(0005829)
Torr Samaho (administrator)
2013-01-27 08:30

Using an std:set is more complicated than using a bool. Nevertheless, I agree that it's still simple enough and changed the warning code accordingly. It's just using the ActorNetworkIndex instead of the actor's name.

(0005830)
Zalewa (developer)
2013-01-27 08:31

Thanks, I'll test it once the next build comes up.

(0006204)
Arco (updater)
2013-04-03 19:44

It appears that this bug is fixed. I tested the wad with the latest version on my own server and it’s not causing any conflict that’s otherwise major. The only thing that I did noticed was a DECORATE offset at 14.

This issue is already marked as resolved. If you feel that is not the case, please reopen it and explain why.
Supporters:	No one explicitly supports this issue yet.
Opponents:	No one explicitly opposes this issue yet.

Issue History
Date Modified	Username	Field	Change
2012-12-02 23:54	Zalewa	New Issue
2012-12-02 23:54	Zalewa	File Added: randomguy.wad
2012-12-03 14:07	Dusk	Status	new => confirmed
2012-12-03 14:10	Dusk	Note Added: 0005465
2012-12-03 15:04	ZzZombo	Note Added: 0005466
2012-12-03 16:29	Zalewa	Note Added: 0005467
2012-12-03 16:43	Dusk	Note Added: 0005468
2012-12-03 16:59	ZzZombo	Note Added: 0005469
2012-12-07 21:31	Dark-Assassin	Note Added: 0005481
2013-01-13 10:47	Torr Samaho	Note Added: 0005759
2013-01-13 11:00	Zalewa	Note Added: 0005760
2013-01-13 11:07	Zalewa	Note Edited: 0005760	View Revisions
2013-01-13 11:18	Torr Samaho	Note Added: 0005761
2013-01-13 11:19	Torr Samaho	Note Edited: 0005761	View Revisions
2013-01-13 11:20	Torr Samaho	Assigned To	=> Torr Samaho
2013-01-13 11:20	Torr Samaho	Status	confirmed => needs testing
2013-01-13 11:20	Torr Samaho	Target Version	=> 1.1
2013-01-13 11:28	Zalewa	Note Added: 0005762
2013-01-13 11:31	Torr Samaho	Note Added: 0005763
2013-01-16 17:56	Dusk	Relationship added	has duplicate 0001252
2013-01-17 01:01	Blzut3	Note Added: 0005795
2013-01-17 01:06	Blzut3	Note Edited: 0005795	View Revisions
2013-01-17 01:11	Dusk	Note Added: 0005796
2013-01-26 13:51	Zalewa	Note Added: 0005825
2013-01-26 13:51	Zalewa	File Added: randomguy_and_randomguy2.wad
2013-01-26 19:56	Torr Samaho	Note Added: 0005826
2013-01-26 20:15	Zalewa	Note Added: 0005827
2013-01-26 20:16	Zalewa	Note Edited: 0005827	View Revisions
2013-01-27 08:30	Torr Samaho	Note Added: 0005829
2013-01-27 08:31	Zalewa	Note Added: 0005830
2013-04-03 19:44	Arco	Note Added: 0006204
2013-04-05 19:52	Dusk	Status	needs testing => resolved
2013-04-05 19:52	Dusk	Fixed in Version	=> 1.1
2013-04-05 19:52	Dusk	Resolution	open => fixed
2013-08-02 10:41	Dusk	Relationship added	related to 0001162
2014-06-14 11:01	Dusk	Relationship added	related to 0001832
2018-09-30 20:16	Blzut3	Status	resolved => closed

Notes
(0005465) Dusk (developer) 2012-12-03 14:10	This is the see state: See: TROO A 0 A_Jump(256,"vile","head","skel","boss","imp","spid","fat") goto see The client does not perform the jump until the server instructs it to, so it keeps looping. The stack is overflown before the client gets the order to jump.

(0005466) ZzZombo (reporter) 2012-12-03 15:04	Maybe use "wait" instead of "goto See"?

(0005467) Zalewa (developer) 2012-12-03 16:29	ZzZombo: In my opinion if something works correctly offline then it should also work correctly online. No excuses about "WAD being bad".

(0005468) Dusk (developer) 2012-12-03 16:43	It's a crash and should be addressed...

(0005469) ZzZombo (reporter) 2012-12-03 16:59	I just provide a possible workaround to the problem that can bu used unless you will make a patch, that's the point.

(0005481) Dark-Assassin (administrator) 2012-12-07 21:31	For online use, at least 1 tic is needed for A_Jump loops. This code would have to be modified to compensate.

(0005759) Torr Samaho (administrator) 2013-01-13 10:47	As Dusk pointed out, the see state is not compatible with Zandronum's jump handling and I don't see any proper way to fix this without completely throwing away Carn's jump handling. As workaround we could simply try to break infinite loops on the clients (always risking to break valid but very long state combinations of zero duration).

(0005760) Zalewa (developer) 2013-01-13 11:00 edited on: 2013-01-13 11:07	Quote from "Torr Samaho" As workaround we could simply try to break infinite loops on the clients (always risking to break valid but very long state combinations of zero duration). This looks reasonable enough. After all, the clients should synchronize with server eventually (I hope they will) so the player would only see a desynch comparable to what happens with Lost Souls now. This is definitely better than a crash that freezes some computers so strongly that the only option left is to press the reset button. Furthermore a warning could be printed to console when this happens, preferably only once per single game/map, so the modder knows that something is wrong with his decorate.

(0005761) Torr Samaho (administrator) 2013-01-13 11:18 edited on: 2013-01-13 11:19	Since anything is better than a crash, clients now try to break infinite state loops of zero duration by not allowing any actor to go through more than 10000 states in one tic. This should fix the problems with randomguy. Since this is a very ugly workaround, a warning is printed every time the workaround kicks in. EDIT: Posted this without seeing your latest response. Printing the warning only once could be a good compromise.

(0005762) Zalewa (developer) 2013-01-13 11:28	I've tested your build and it appears to work correctly now.

(0005763) Torr Samaho (administrator) 2013-01-13 11:31	Thanks for checking! The client side infinite loop workaround warning is now only printed once on each Zandronum instance. Would be nice if somebody confirms this change in the upcoming official 1.1 beta build.

(0005795) Blzut3 (administrator) 2013-01-17 01:01 edited on: 2013-01-17 01:06	One thing to consider is it's probably fairly common to use A_Jump(256, ...) for jumping to some random state. Any states following these calls will never be run, so perhaps it might be wise to change the client version of A_Jump to set the state duration to -1 if maxchance >= 256 so it waits for the server information? Edit: While on the topic of knowns in A_Jump. A_Jump(256, "SomeState") is sometimes used instead of 'goto SomeState' since it is resolved at runtime instead of "compile time." This would be another fairly simple condition to check if(maxchance >= 256 && count == 1) and the client would accurately predict this case.

(0005796) Dusk (developer) 2013-01-17 01:11	I like that, though I think that should be taken to a new ticket.

(0005825) Zalewa (developer) 2013-01-26 13:51	Tested with build "1.1-alpha-r130120-0905". Two things I've noticed: 1. Reloading map with 'map' command doesn't reset the 'message was displayed' flag. In other words: to see the message again you have to close the process of the server and start a new one. This might be the intended behaviour, although. 2. When there are multiple offending actor types the message is displayed only for the first offender. Subsequent errors, even for other actors, are ignored silently. As an example I copied entire "RandomGuy" code and pasted it into "RandomGuy2". Summoning RandomGuy displays the message, but if RandomGuy2 is summoned next then the message is not displayed. Reverse summoning order yields the same result. I'm attaching WAD with these actors.

(0005826) Torr Samaho (administrator) 2013-01-26 19:56	That's the behavior I intended and what I meant with "is only printed once on each Zandronum instance" in the log. Printing it once per instance and per actor type would probably be nicer, but also is more complicated to implement. I decided to go with the simple implementation to have more time to work on other issues.

(0005827) Zalewa (developer) 2013-01-26 20:15 edited on: 2013-01-26 20:16	Quote from "Torr Samaho" That's the behavior I intended and what I meant with "is only printed once on each Zandronum instance" in the log. Roger that. Quote from "Torr Samaho" Printing it once per instance and per actor type would probably be nicer, but also is more complicated to implement. I decided to go with the simple implementation to have more time to work on other issues. I disagree on the complicated part. std::set<std::string> actors; if (actors.find(actorType) == actors.end()) { actors.insert(actorType); printWarning(); } I don't consider this fixed until it treats each actor type individually.

(0005829) Torr Samaho (administrator) 2013-01-27 08:30	Using an std:set is more complicated than using a bool. Nevertheless, I agree that it's still simple enough and changed the warning code accordingly. It's just using the ActorNetworkIndex instead of the actor's name.

(0005830) Zalewa (developer) 2013-01-27 08:31	Thanks, I'll test it once the next build comes up.

(0006204) Arco (updater) 2013-04-03 19:44	It appears that this bug is fixed. I tested the wad with the latest version on my own server and it’s not causing any conflict that’s otherwise major. The only thing that I did noticed was a DECORATE offset at 14.