Skip to content
This repository was archived by the owner on Jan 12, 2022. It is now read-only.

Debian crash fix + Game handle leak fix#10

Closed
Kenzzer wants to merge 2 commits intopowerlord:masterfrom
Kenzzer:master
Closed

Debian crash fix + Game handle leak fix#10
Kenzzer wants to merge 2 commits intopowerlord:masterfrom
Kenzzer:master

Conversation

@Kenzzer
Copy link

@Kenzzer Kenzzer commented Aug 18, 2021

Considering those fixes are important, especially the debian crash fix, it'd be great if this PR could be merged. As this repository remains the main source where server operators obtain their copy of nativevotes.

nosoop and others added 2 commits July 10, 2019 02:03
Increments the refcount of the private forward.  This prevents a crash on Debian 10 and doesn't seem like well-defined behavior.
@Kenzzer
Copy link
Author

Kenzzer commented Aug 18, 2021

I also just realised that PR #9 exists, and while I was about to close my PR.
I do realise that their fix for handle leak is :
optionsEvent.Cancel();
while it should be :
delete optionsEvent;

As the description of cancel native is as follow :
Cancels a previously created game event that has not been fired.
Reference : https://sm.alliedmods.net/new-api/events/Event/Cancel

However our event, is fired by nativevotes, it is therefore better to use delete keyword from a technical standpoint. And we also avoid potential behaviour change, should SM decides in future versions to throw an error if Cancel is called on a fired event.

@sapphonie
Copy link

sapphonie commented Aug 26, 2021

I don't think it makes a diff using delete; vs .Cancel? The api specifically mentioned it frees handles lol.

And why did you make a PR with other people's changes? Just open an issue

Also, I've been maintaining https://github.com/sapphonie/sourcemod-nativevotes-updated as an aggregated "maintained" version of nativevotes, including these changes + updates for 1.10 syntax and other updates for MvM, using the PRs from this repo + people's forks. Just use that

@Kenzzer
Copy link
Author

Kenzzer commented Aug 26, 2021

I don't think it makes a diff using delete; vs .Cancel? The api specifically mentioned it frees handles lol.

While it does mention that it frees the event, the api also notes it's for non fired event :
https://github.com/alliedmodders/sourcemod/blob/1.10-dev/plugins/include/events.inc#L91
Now of course internally, SM might not care at all. This PR's approach was to semantically correct with the API.
I also re-iterate that I did not see your PR originally, and opened this one withour prior checks.

And why did you make a PR with other people's changes? Just open an issue
Nosoop's nativevotes, is currently the autority among the tf2 communities I work with. (Namely redsun.tf, potato.tf, glub's servs, and dfs.tf). My fix was therefore applied to his fork, rather than branching yet again off this repository.

However, if you wish to have your repo as the head, then I've no issue with this and will gladly close this PR!
I've a hard time reaching powerlord through steam anyways.

@Kenzzer Kenzzer closed this Aug 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants