asherkin / TF2Items

Items with custom attributes.
https://forums.alliedmods.net/forumdisplay.php?f=146
32 stars 10 forks source link

Extention not freeing override handle during TF2Items_OnGiveNamedItem #7

Closed JoinedSenses closed 5 years ago

JoinedSenses commented 5 years ago

https://github.com/asherkin/TF2Items/blob/d81b0ff7e98b197431ff13f1a16a6989c5ae2032/extension/extension.cpp#L184

Is it intentional that the extension does not free the handle of the overridden item during TF2Items_OnGiveNamedItem ? There isn't any documentation on the intentions here. My workaround is to create a timer and free the handle:

public Action TF2Items_OnGiveNamedItem(int client, char[] classname, int weapon, Handle &override) {
    /* ... */
    override = TF2Items_CreateItem(OVERRIDE_ATTRIBUTES);
    /* ... */

    if (override) {
        CreateTimer(1.0, timerDelete, override);
        return Plugin_Changed;
    }
}

Action timerDelete(Handle timer, Handle item) {
    delete item;
}
asherkin commented 5 years ago

Yes - this bit of confusing design came from the original built-in implementation of tf2items_manager, as it pre-creates all the override handles when parsing the config file. The normal SM handle ownership rules apply, your plugin creates the handle and TF2Items_OnGiveNamedItem isn't explicitly documented as taking ownership so it is your plugin's responsibility to delete the handle. A common approach is to store the handle globally and free it on the next call to TF2Items_OnGiveNamedItem, but a timer should work fine.

geominorai commented 5 years ago

How long should the timer wait before freeing the handle? Would there be a problem if it inadvertently frees it before the next call to TF2Items_OnGiveNamedItem?

asherkin commented 5 years ago

It doesn't matter, the contents of the handle is only used by the code invoking the forward, so as the VM is single-threaded, by the time anything can call into SourcePawn again it is safe to delete it.

geominorai commented 5 years ago

Good to know, thanks!