Issue information

Issue ID
#6804
Status
Fixed
Severity
None
Started
Hercules Elf Bot
Oct 19, 2012 21:27
Last Post
Hercules Elf Bot
Oct 29, 2012 15:17
Confirmation
Yes (1)
No (0)

Hercules Elf Bot - Oct 19, 2012 21:27

Originally posted by [b]AnnieRuru[/b]
http://rathena.org/board/topic/72518-monster-hunt-event/page__st__20#entry147889

[code] // immediately copy $@partymembercount value to a new variable, since
// you don't know when 'getpartymember' will get called again for someone
// else's party, overwriting your global array.
set @partymembercount,$@partymembercount;[/code]this is really lol
not only it use temporary player variable
it also creates a rumor that 2 people talking to this npc can clash the $@partymembercount variable

that is totally a rumor
with current script engine
please take a look at what ultramage and mirei said

http://www.eathena.ws/board/index.php?autocom=bugtracker&showbug=146
[quote name='theultramage']The npc will execute on the first player until it arrives in a "waiting for input" or "end" state, like hitting a next; label. Then it'll switch.
Until then, all CPU power is dedicated to executing the script (this is why you shouldn't make long-running scripts, it'll lag players).
[b]This also means no milisecond-like race conditions can happen[/b] (but you can still get bad results if you don't plan your next;s properly).[/quote]

http://www.eathena.ws/board/index.php?autocom=bugtracker&showbug=4935
[quote name='Ai4rei']When there is no *sleep, *sleep2, *next, *close2, *input, *select, *prompt, *menu or any other script command, that pauses the script, between *getpartymember and *copyarray, [b]the operation is atomic and no cross-script exploits can occur, as there is always only one script running at the same time.[/b][/quote]

so please change it don't have to set another variable anymore, it really cause misunderstanding

This post has been edited by AnnieRuru on Oct 19, 2012 21:29

Hercules Elf Bot - Oct 24, 2012 8:51

Originally posted by [b]MarkZD[/b]
premiseIt's a thing that should be carefully looked at the code, not just on incomplete guesses.
[quote]The npc will execute on the first player until it arrives in a "waiting for input" or "end" state, like hitting a next;[/quote]
If it's at the same time, there's no FIRST player, both are the same.


[quote][b]This also means no milisecond-like race conditions can happen[/b][/quote]
It's not about milisecond race, it's about the same time.


[quote][b]the operation is atomic and no cross-script exploits can occur, as there is always only one script running at the same time.[/b][/quote]
Atomic operation is fully dependent on the OS, Hardware and the script engine system.
Also, atomic operation is usually meant to adding something, not changing it. (As a text to a file, because it's wrote to different blocks etc).

Technically, there's no atomic operation for a simple var, talking on hardware level, as the hardware has control over what segment will be changed, and it's determined by the cpu operation, so in this case it wouldn't conflict, because the next cicle will always happen after the first.
But, to check this particular case, we need to verify the script engine.

This is why there's lock and semaphore system around a lot of other system.
I already used system which writing to the same variable at same time would deal to broken data.

This is not really a bug, this is more like an improvement to the doc(maybe, if the doc is right, it'll become a bug with this change).
[u]But with this premise, even with this additional assurance it could bug.[/u]

And if there's really some lack of lock on var data on the script engine, so, maybe it should be revised, as it can cause bugs.(Of course we may think it doesn't happen as it's very unlikely two request will arrive at the same time, but who knows)?

Just my opnion(But, as I said before, it's something we should look at source code, not just over some guesses).

A good way to check would be creating 2 or more scripts to change same var on a loop and see what happen, possibly will all be okay, but I like to check on everything, however, it won't simulate user clicking on a NPC, as it may have some kind of queue control preventing the descripted bugs, caused by the user side.

This post has been edited by MarkZD on Oct 24, 2012 10:01

Hercules Elf Bot - Oct 25, 2012 2:47

Originally posted by [b]AnnieRuru[/b]
yes, this is NOT a bug,
if you click on those 2 bug report link I stated above
both of them are mark as 'Working as Intended'

its the documentation that's provide false fact,
leads members to believe our script engine able to override other script instance
which in this document, its a wrong point

totally nothing wrong with the source code or script commands


anyway, since this one has standing here for some time
I guess I write it in my own sentence

[codebox]Example:

set .register_num, 5; // set this event need 5 party members to start

// get the charID and accountID of character's party ID
getpartymember getcharid(1), 1;
getpartymember getcharid(1), 2;

if ( $@partymembercount != .register_num ) {
mes "Please form a party of "+ .register_num +" to continue";
close;
}

// loop through both of them using isloggedin command
for ( set .@i, 0; .@i < $@partymembercount; set .@i, .@i +1 )
if ( isloggedin( $@partymemberaid[.@i], $@partymembercid[.@i] ) )
set .@count_online, .@count_online +1 ;
// the reason to extend the search into character ID is because a single
// party can accept multiple characters from the same account. Without
// searching through the character ID, if that player has 2 characters
// from the same account inside that party, it will count twice.

if ( .@count_online != .register_num ) {
mes "All your party members must online to continue";
close;
}

// copy the array to prevent player cheat the system
copyarray .@partymembercid, $@partymembercid, .register_num;

mes "Are you ready ?";
next; // careful here
select "Yes";

// when a script hit a next, menu, sleep or input that pauses the script
// players can take this chance to invite or /leave to make changes in their party
// and to prevent that, we need to call up getpartymember again

getpartymember getcharid(1), 1;
if ( $@partymembercount != .register_num ) {
mes "You've made changes to your party !";
close;
}
for ( set .@i, 0; .@i < $@partymembercount; set .@i, .@i +1 ) {
if ( .@partymembercid[.@i] != $@partymembercid[.@i] ) {
mes "You've made changes to your party !";
close;
}
}

// finally, its safe to start the event
warpparty "event_map", 0,0, getcharid(1);[/codebox]
try to revise this throughly before you commit though
if you have any question, ask ...

This post has been edited by AnnieRuru on Oct 25, 2012 3:40

Hercules Elf Bot - Oct 29, 2012 9:36

Originally posted by [b]KeyWorld[/b]
Hey Annie <3

I always say : "Why the hell the script engine generate $@var instead of .@var !?".
It's the same problem with all commands that generate $@var and @var.

I don't think the doc has to be change. It's better to say to novice scripter to copy directly the variable to avoid problems instead of explain them when they should copy the variables to avoid overwrite (more difficult to understand when you are new in script).

Hercules Elf Bot - Oct 29, 2012 10:21

Originally posted by [b]MarkZD[/b]
[quote name='AnnieRuru' timestamp='1351133248' post='15150']
Working as Intended'its the documentation that's provide false fact
[/quote]

As we are talking about documentation, if it's a false fact it's an error, so it's a bug, and that's what I'm talking about, the sources you provided don't confirm it's false, they just make guesses about it being, those guesses which I already showed how they could be false.

I'm talking about the quotes you mentioned, I'm just telling it'd only be a trusted "source", if you quoted the source code explaining the algorithm, proving it'd never fail.

You're talking as if it's the perfect true, based on your sources and that's what I'm criticizing.

IMHO.

This post has been edited by MarkZD on Oct 29, 2012 10:22

Hercules Elf Bot - Oct 29, 2012 11:24

Originally posted by [b]Brian[/b]
Not sure if I understood everything MarkZD said, but here is a script (with a [b][wiki='next'][/b] pause) that demonstrates a [b]$@[/b]variable being modified externally:

[codebox]prontera,155,180,0 script test 910,{
mes "I am storing your char_id.";
set $@the_charid, getcharid(0);
mes "$@the_charid = " + $@the_charid;
mes " ";

select("Click this NPC with another char, then <Enter>:Don't click the NPC with another char");

mes "$@the_charid = " + $@the_charid;
close;
}[/codebox]


I added AnnieRuru's example in [rev=16840].

Hercules Elf Bot - Oct 29, 2012 13:39

Originally posted by [b]AnnieRuru[/b]
[quote name='MarkZD' timestamp='1351506099' post='15200']You're talking as if it's the perfect true, based on your sources and that's what I'm criticizing.IMHO.[/quote]
[url=http://www.eathena.ws/board/index.php?showuser=6139]Fredzilla[/url] was the one who started writing script_commands.txt
and eAAC mod at the time added the documentation he did into eathena SVN 5~6 years ago
without Fredzilla, we would never have a script_commands.txt that explained all the script commands we have

and I mean no offense, script_commands.txt at the time he wrote has many lies
just look at eathena's bug tracker and you'll find out a bunch of them

what I mean is, Fredzilla truely does helped many of us, including me when I tried learning how to script
but even his explanation is not perfect
that's why in this community forum, we should point out each other mistakes and make for a better future


about generating $@var into .@var ....
lol ...
if we do that right now, many scripts will broken lmao
at that time, if you look at my old scripts from year 2007, you'll see that I used
for ( set @i, 0; @i < 10; set @i, @i +1 ) <-- this player variable
because at the time all of us haven't learn this new techniques,
and it was us, especially during year 2008, spreading the new scripting techniques,
and we have found many ways to write a more optimized script now

its the script revolutions that changes this system

and yeah, I also hope from now on, when core developers wants to generate a new array
like battleground $@arenamembers <-- still using global var
I also hope we should reminds them to use .@scope_var to generate the array

but I'm not in the development team though, so I couldn't comment much about this


oh and thx Brian xD
status can be set to fix now


EDIT !!!
wait
2365 if ( $@partymembercount < .register_num ) {
2366 mes "Please form a party of "+ .register_num +" to continue";
2367 close;
2368 }

why is that < operator but not != ....
in my example, I use !=, but you changed to <

This post has been edited by AnnieRuru on Oct 29, 2012 14:03

Hercules Elf Bot - Oct 29, 2012 15:17

Originally posted by [b]Brian[/b]
oops I made a typo, thanks Annie.

Fixed in [rev=16841].