Frag1337
commented
2 years ago Very neat addition!
Open CookStar opened 2 years ago
Frag1337
commented
2 years ago Very neat addition!
CookStar
commented
2 years ago 
jordanbriere
commented
2 years ago Works great! Though you forgot a callable check when registering callbacks. The following should raise:
ConVarChanged(ConVar("test", "1", "test convar."))(None)Because None is not callable.
I haven't investigated as to why, but the following crashes on reload:
from commands.server import ServerCommand
from cvars import ConVar
from cvars import ConVarChanged
@ServerCommand('test_command')
def test_command(*args):
...
@ConVarChanged(ConVar('test_command'))
def on_test_command(convar, old_value, new_value):
...Likely because the associated ICvar is freed by the server commands manager while still being held by the interpreter. Raising if cvar.find_command(command) should suffice, though.
Also, would not this be better to extend OnConVarChanged and CConVarChangedListenerManager directly instead of having a separated decorator as well as a duplicated global callback? For example:
@OnConVarChanged
def on_test_command(convar, old_value, new_value):
"""Called when any convar changes."""
@OnConVarChanged('test')
def on_test_command(convar, old_value, new_value):
"""Called when test changes."""I think this could be best, as it currently feel like reinventing the wheel to me.
CookStar
commented
2 years ago Works great! Though you forgot a callable check when registering callbacks. The following should raise:
ConVarChanged(ConVar("test", "1", "test convar."))(None)Because
Noneis not callable.
Added callable confirmation.
Likely because the associated
ICvaris freed by the server commands manager while still being held by the interpreter. Raising ifcvar.find_command(command)should suffice, though.
test_command is not a valid ConVar in the first place, so it should not be registered. I've added a check for ConVar. We could also add a check to ConVarExt::AddChangedCallback, but I think that should be done in ConVarExt::__init__.
Also, would not this be better to extend
OnConVarChangedandCConVarChangedListenerManagerdirectly instead of having a separated decorator as well as a duplicated global callback?
If we can compromise the backward compatibility of OnConVarChanged, I will do so. I also thought about it but gave up because OnConVarChanged doesn't have new_value. (new_value should have been there from the beginning.)
CookStar
commented
2 years ago
test_commandis not a valid ConVar in the first place, so it should not be registered. I've added a check for ConVar. We could also add a check toConVarExt::AddChangedCallback, but I think that should be done inConVarExt::__init__.
Prevented the creation of invalid ConVars. https://github.com/Source-Python-Dev-Team/Source.Python/pull/421/commits/080d8cad384ace6658b128f508a46f6778755276
jordanbriere
commented
2 years ago
test_commandis not a valid ConVar in the first place, so it should not be registered. I've added a check for ConVar. We could also add a check toConVarExt::AddChangedCallback, but I think that should be done inConVarExt::__init__.
You cannot override a command with a convar due to how the internal linking of the engine works but the other way around should be possible. I think the problem is actually there:
It should be:
// Get the ConCommand instance
ConCommand* pConCommand = g_pCVar->FindCommand(m_Name);
if (pConCommand) {
// Unregister the ConCommand
g_pCVar->UnregisterConCommand(pConCommand);
}Because FindCommand returns NULL, now that m_Name is a ConVar, and unregistering a NULL pointer is causing an access violation due to the lack of sanity checks.
EDIT: Fixed into https://github.com/Source-Python-Dev-Team/Source.Python/commit/1604995f637973c6676502ca66535351a3acfca7.
If we can compromise the backward compatibility of OnConVarChanged, I will do so. I also thought about it but gave up because OnConVarChanged doesn't have
new_value. (new_valueshould have been there from the beginning.)
I would rather discard new_value altogether and leave it up to the callbacks to cast the convar as the type they want to work with. This would ensure they always work with the current value in case a previous callback changed it to something else before they get executed while also enforcing behavioral consistency. For example, bool(ConVar('test', 'some random value')) is False, but bool('some random value') is True, float(ConVar('test', 'some random value')) is 0.0, but float('some random value') produces a ValueError, etc.
CookStar
commented
2 years ago You cannot override a command with a convar due to how the internal linking of the engine works but the other way around should be possible.
No it won't work, ConVar and ConCommand are incompatible. We need to add a check to ConCommand just as I added a check at ConVarExt::__init__.
Code:
from commands.server import ServerCommand
from cvars import ConVar
from cvars import cvar
test_command = ConVar("test_command")
print("ConVar - test_command:", type(cvar.find_var("test_command")))
print("ConCommand - test_command:", type(cvar.find_command("test_command")))
@ServerCommand('test_command')
def test_command(*args):
print("ConCommand test_command")
print("ConVar - test_command:", type(cvar.find_var("test_command")))
print("ConCommand - test_command:", type(cvar.find_command("test_command")))Output:
ConVar - test_command: <class '_cvars.ConVar'>
ConCommand - test_command: <class 'NoneType'>
WARNING: unable to link test_command and test_command because one or more is a ConCommand.
ConVar - test_command: <class '_cvars.ConVar'>
ConCommand - test_command: <class 'NoneType'>
test_command
"test_command" = ""Because
FindCommandreturnsNULL, now thatm_Nameis aConVar, and unregistering aNULLpointer is causing an access violation due to the lack of sanity checks.
And I don't think this fix(1604995) is necessary if the ConCommand is valid.
I would rather discard
new_valuealtogether and leave it up to the callbacks to cast the convar as the type they want to work with. This would ensure they always work with the current value in case a previous callback changed it to something else before they get executed while also enforcing behavioral consistency.
The current implementation is broken, but that's exactly why we need new_value. OnConVarChanged/ConVarChanged is used in the majority of cases, either as a Listener, Override, or Warning. In particular, if you want to issue a warning, you cannot compare the current value with the input value(new_value) just by looking convar, which will make the behavior inconsistent.
Example: test1/test1.py
from cvars import ConVarChanged
@ConVarChanged("bot_quota")
def on_bot_quota(convar, old_value):
convar.set_int(1)test2/test2.py
from cvars import ConVarChanged
@ConVarChanged("bot_quota")
def on_bot_quota(convar, old_value):
new_value = convar.get_int()
if new_value < 1 or new_value > 5:
print("bot_quota maximum value is 5.")
convar.set_int(5)Output1
sp plugin load test1
sp plugin load test2
bot_quota 10
bot_quota
"bot_quota" = "1" ( def. "10" ) game replicated
- Determines the total number of bots in the game.Output2
sp plugin load test2
sp plugin load test1
bot_quota 10
bot_quota maximum value is 5.
bot_quota
"bot_quota" = "1" ( def. "10" ) game replicated
- Determines the total number of bots in the game.It is not good to have the behavior change depending on the loading order of the plugins, and such a warning should be useful when debugging. new_value will solve this problem.
Also, if ConVarChanged is available, OnConVarChanged should only be used in very limited situations. Wouldn't it be better to deprecate OnConVarChanged and move to ConVarChanged? Either way, I'll refactor the code and add commit.
jordanbriere
commented
2 years ago No it won't work, ConVar and ConCommand are incompatible. We need to add a check to ConCommand just as I added a check at
ConVarExt::__init__.Code:
from commands.server import ServerCommand from cvars import ConVar from cvars import cvar test_command = ConVar("test_command") print("ConVar - test_command:", type(cvar.find_var("test_command"))) print("ConCommand - test_command:", type(cvar.find_command("test_command"))) @ServerCommand('test_command') def test_command(*args): print("ConCommand test_command") print("ConVar - test_command:", type(cvar.find_var("test_command"))) print("ConCommand - test_command:", type(cvar.find_command("test_command")))Output:
ConVar - test_command: <class '_cvars.ConVar'> ConCommand - test_command: <class 'NoneType'> WARNING: unable to link test_command and test_command because one or more is a ConCommand. ConVar - test_command: <class '_cvars.ConVar'> ConCommand - test_command: <class 'NoneType'> test_command "test_command" = ""
You are trying to override a ConVar with a ConCommand which, as stated in the sentence you quoted, cannot be done because the engine doesn't allow such linking (as shown in that warning message). However, the other way around (ConCommand → ConVar), is possible. For example:
from commands.server import ServerCommand
from cvars import cvar
from cvars import ConVar
from engines.server import execute_server_command
@ServerCommand('test_command')
def test_command(*args):
print("ConCommand test_command")
assert cvar.find_var('test_command') is None
assert cvar.find_command('test_command') is not None
print('test_command is a command as expected...')
execute_server_command('test_command')
test_command = ConVar('test_command', 'this is the value of test_command')
assert cvar.find_command('test_command') is None
assert cvar.find_var('test_command') is not None
print('test_command is now a convar as expected...')
try:
execute_server_command('test_command')
except ValueError:
print('test_command is not a command anymore and cannot be executed...')
print(f'test_command is set to: {str(test_command)}')[SP] Loading plugin 'testing'...
test_command is a command as expected...
ConCommand test_command
test_command is now a convar as expected...
test_command is not a command anymore and cannot be executed...
test_command is set to: this is the value of test_command
[SP] Successfully loaded plugin 'testing'.https://github.com/Source-Python-Dev-Team/Source.Python/commit/080d8cad384ace6658b128f508a46f6778755276 prevents such behaviour because ConVar('test_command') will raise instead of overriding the existing ConCommand.
And I don't think this fix(1604995) is necessary if the ConCommand is valid.
It is, because even if we don't override it, another plugin can do so. We only want to unregister valid commands that were created by us and are still valid.
CookStar
commented
2 years ago However, the other way around (
ConCommand→ConVar), is possible. For example:
It doesn't work on my end.
Output:
sp plugin load test
[SP] Loading plugin 'test'...
Tried to look up command test_command as if it were a variable.
test_command is a command as expected...
ConCommand test_command
Tried to look up command test_command as if it were a variable.
WARNING: unable to link test_command and test_command because one or more is a ConCommand.
[SP] Caught an Exception:
Traceback (most recent call last):
File "../addons/source-python/packages/source-python/plugins/command.py", line 164, in load_plugin
plugin = self.manager.load(plugin_name)
File "../addons/source-python/packages/source-python/plugins/manager.py", line 207, in load
plugin._load()
File "../addons/source-python/packages/source-python/plugins/instance.py", line 74, in _load
self.module = import_module(self.import_name)
File "../addons/source-python/plugins/test/test.py", line 18, in <module>
assert cvar.find_command('test_command') is None
AssertionError
[SP] Plugin 'test' was unable to be loaded.Edit: New Test
from commands.server import ServerCommand
from cvars import cvar
from cvars import ConVar
from engines.server import execute_server_command
@ServerCommand('test_command')
def test_command(*args):
print("ConCommand test_command")
print("ConVar - test_command:", type(cvar.find_var("test_command")))
print("ConCommand - test_command:", type(cvar.find_command("test_command")))
execute_server_command('test_command')
test_command = ConVar('test_command', 'this is the value of test_command')
print("ConVar - test_command:", type(cvar.find_var("test_command")))
print("ConCommand - test_command:", type(cvar.find_command("test_command")))
try:
execute_server_command('test_command')
except ValueError:
print('test_command is not a command anymore and cannot be executed...')Output:
sp plugin load test
[SP] Loading plugin 'test'...
Tried to look up command test_command as if it were a variable.
ConVar - test_command: <class 'NoneType'>
ConCommand - test_command: <class '_commands._server.ServerCommandDispatcher'>
ConCommand test_command
Tried to look up command test_command as if it were a variable.
WARNING: unable to link test_command and test_command because one or more is a ConCommand.
Tried to look up command test_command as if it were a variable.
ConVar - test_command: <class 'NoneType'>
ConCommand - test_command: <class '_commands._server.ServerCommandDispatcher'>
ConCommand test_command
[SP] Successfully loaded plugin 'test'.It doesn't work on my end.
What game? It's possible different engine version behave differently. I'm testing on CS:S.
CookStar
commented
2 years ago It doesn't work on my end.
What game? It's possible different engine version behave differently. I'm testing on CS:S.
CS:GO
CookStar
commented
2 years ago Since sourcemod was designed to avoid overlapping ConVar, ConCommand, I assumed it would be incompatible with any game.
ConVar: https://github.com/alliedmodders/sourcemod/blob/master/core/ConVarManager.cpp#L407
ConCommand(ServerCommand): https://github.com/alliedmodders/sourcemod/blob/master/core/smn_console.cpp#L754
jordanbriere
commented
2 years ago So, when you do the following:
cmd = ServerCommand('foo')(lambda *args: None)
var = ConVar('foo')The behaviours are essentially the same on all games; both the command and the variable get registered. However, the resolution is different. CS:S uses a linked list that resolves from rtl (newer first) while CS:GO (as well as Blade and L4D2) uses a hash list that resolves from ltr (older first). Basically, the following:
base = cvar.find_base('foo')Returns var on CS:S and cmd on CS:GO. I originally thought var was overwriting cmd in the global list, but that isn't the case. The other way around, if we were to allow the linking, would have the same results:
var = ConVar('foo')
cmd = ServerCommand('foo')(lambda *args: None)Because we register everything that is queried no question asked:
I will look more into that likely around the week end.
jordanbriere
commented
2 years ago At the end I had time to look more into it and came up with the following that addresses the registration conflicts as well as some other fixes: https://github.com/Source-Python-Dev-Team/Source.Python/commit/02e34907fc86036228d132526edcd8f847a0650a
Here is the code I used to test on CS:S and CS:GO:
from commands.server import ServerCommand
from cvars import ConVar
from cvars import cvar
from engines.server import execute_server_command
from memory import get_object_pointer
from gc import collect
collect()
sv_cheats = ConVar('sv_cheats')
sv_cheats.set_bool(False)
@ServerCommand('sv_cheats')
def callback(command):
print('sv_cheats cannot be assigned from the console!')
return False
execute_server_command('sv_cheats', '1')
assert not bool(sv_cheats)
assert get_object_pointer(ConVar('sv_cheats')) == get_object_pointer(sv_cheats)
assert not cvar.find_var('sv_cheats').is_command()
cmd = cvar.find_command('sv_cheats')
assert cmd.is_command()
cvar.unregister_base(cmd)
assert cvar.find_base('sv_cheats') is None
cvar.register_base(sv_cheats)
assert cvar.find_var('sv_cheats') is not None
assert cvar.find_command('sv_cheats') is None
assert get_object_pointer(cvar.find_base('sv_cheats')) == get_object_pointer(sv_cheats)
try:
execute_server_command('sv_cheats', '1')
raise AssertionError
except ValueError:
pass
assert cvar.find_var('foo') is None
foo = ConVar('foo', 'foo')
assert cvar.find_var('foo') is not None
@ServerCommand('foo')
def callback(command):
if command.arg_string == 'baz':
return
execute_server_command('foo', 'baz')
return False
assert str(foo) == 'foo'
execute_server_command('foo', 'bar')
assert str(foo) == 'baz'
del foo
collect()
assert cvar.find_var('foo') is None
foo = ConVar('foo')
assert not foo.is_registered()
print('All assertions passed!')
Also, if ConVarChanged is available, OnConVarChanged should only be used in very limited situations. Wouldn't it be better to deprecate OnConVarChanged and move to ConVarChanged?
I agree that most uses are currently testing for the convar they are looking for, however, I'm strongly against deprecating features or breaking backward compatibility unless absolutely necessary. Having separate decorators also seems redundant to me, especially when extending the current classes to offer the proposed features is not that difficult. The behaviours of the decorator can be achieved with something like that:
from cvars import ConVar
class OnConVarChanged:
def __init__(self, callback, *convars):
if isinstance(callback, (str, ConVar)):
convars = (callback,) + convars
callback = None
elif not callable(callback):
raise ValueError('The given callback is not callable.')
self.convars = convars
self.callback = callback
if not convars:
print(f'{callback.__name__} will be called for any convar.')
def __call__(self, callback):
assert self.convars, 'No convars defined on initialization.'
self.callback = callback
print(f'{callback.__name__} will be called for these convars: {self.convars}')
return self
@OnConVarChanged
def on_any_convar_changed():
...
@OnConVarChanged('test', 'test2', ConVar('test3'))
def on_specified_convars_changed():
...on_any_convar_changed will be called for any convar.
on_specified_convars_changed will be called for these convars: ('test', 'test2', <_cvars.ConVar object at 0x20413B40>)but that's exactly why we need
new_value
We could really just override CConVarChangedListenerManager::Notify and look up if the callbacks take 2 or 3 arguments and call them accordingly. For example, the required arguments count can be obtained with something like this:
#include "code.h"
int get_argcount(object function)
{
object code;
try {
code = getattr(function, "__func__", function).attr("__code__");
} catch (error_already_set &) {
if (!PyErr_ExceptionMatches(PyExc_AttributeError))
throw_error_already_set();
PyErr_Clear();
return -1;
}
if (extract<int>(code.attr("co_flags")) & CO_VARARGS) {
return BOOST_PYTHON_MAX_ARITY;
}
int nArgCount = extract<int>(code.attr("co_argcount")) + extract<int>(code.attr("co_kwonlyargcount"));
return PyObject_HasAttrString(function.ptr(), "__self__") ? nArgCount - 1 : nArgCount;
}print('"lambda a, b, *, c=3: None" =>', get_argcount(lambda a, b, *, c=3: None))
def function(a, b): ...
print('"def function(a, b): ..." =>', get_argcount(function))
def function(*args): ...
print('"def function(a, b, c, *args): ..." =>', get_argcount(function))
class Foo:
def method(self, a, b, c=None): ...
@staticmethod
def staticmethod(a, b, c): ...
print('"Foo.method(self, a, b, c=None): ..." =>', get_argcount(Foo.method))
print('"Foo.staticmethod(a, b, c): ..." =>', get_argcount(Foo.staticmethod))
print('"Foo().method(self, a, b, c=None): ..." =>', get_argcount(Foo().method))
print('"Foo().staticmethod(a, b, c): ..." =>', get_argcount(Foo().staticmethod))
print('"None" => ', get_argcount(None))"lambda a, b, *, c=3: None" => 3
"def function(a, b): ..." => 2
"def function(a, b, c, *args): ..." => 32
"Foo.method(self, a, b, c=None): ..." => 4
"Foo.staticmethod(a, b, c): ..." => 3
"Foo().method(self, a, b, c=None): ..." => 3
"Foo().staticmethod(a, b, c): ..." => 3
"None" => -1That info could be cached in your maps so it doesn't have to be retrieved every calls but yeah, I really don't see the necessity of having a separated decorator, deprecating the existing one, or breaking compatibility when it can be worked around rather easily to accommodate both.
CookStar
commented
2 years ago At the end I had time to look more into it and came up with the following that addresses the registration conflicts as well as some other fixes: 02e3490
I understand your intentions, but I have a question, why?
And, there are multiple problems with this implementation, foremost this does not resolve the conflicts themselves.
Code: test1/test1.py
from cvars import cvar
from cvars import ConVar
test_command = ConVar('test_command', 'this is the value of test_command')
assert cvar.find_var('test_command') is not Nonetest2/test2.py
from commands.server import ServerCommand
from cvars import cvar
@ServerCommand('test_command')
def test_command(*args):
print("ConCommand test_command")
assert cvar.find_command('test_command') is not NoneOutput 1:
sp plugin load test1
[SP] Loading plugin 'test1'...
[SP] Successfully loaded plugin 'test1'.
sp plugin load test2
[SP] Loading plugin 'test2'...
[SP] Successfully loaded plugin 'test2'.Output 2:
sp plugin load test2
[SP] Loading plugin 'test2'...
[SP] Successfully loaded plugin 'test2'.
sp plugin load test1
[SP] Loading plugin test1'...
[SP] Caught an Exception:
Traceback (most recent call last):
File "../addons/source-python/packages/source-python/plugins/command.py", line 164, in load_plugin
plugin = self.manager.load(plugin_name)
File "../addons/source-python/packages/source-python/plugins/manager.py", line 207, in load
plugin._load()
File "../addons/source-python/packages/source-python/plugins/instance.py", line 74, in _load
self.module = import_module(self.import_name)
File "../addons/source-python/plugins/test1/test1.py", line 6, in <module>
assert cvar.find_var('test_command') is not None
AssertionError
[SP] Plugin 'test1' was unable to be loaded.If consistency is compromised by the loading order of plugins, then ConVat and ConCommand conflicts should not be allowed from the start. And even if we want to detect changes from the console, we should add a dedicated feature to ConVar, and not allow conflicts with ConCommand.
Second, why did you make it possible to delete ConVar? This makes it very easy to cause a crash.
Code:
from cvars import cvar
from cvars import ConVar
test_command = ConVar('test_command', 'this is the value of test_command')
test_command2 = cvar.find_var('test_command')
assert test_command2 is not None
del test_command
test_command2.set_string('new value of test_command')Output:
sp plugin load test1
[SP] Loading plugin 'test1'...
Thread 1 "srcds_linux" received signal SIGSEGV, Segmentation fault.
0xf076e545 in boost::python::objects::polymorphic_id_generator<ConVar>::execute(void*) () from /home/steam/csgo_ds/csgo/addons/source-python/bin/core.so
=> 0xf076e545 <_ZN5boost6python7objects24polymorphic_id_generatorI6ConVarE7executeEPv+37>: 8b 76 04 mov esi,DWORD PTR [esi+0x4]There is no guarantee that the ConVar is retained by Source.Python, so it should not be able to be deleted.
jordanbriere
commented
2 years ago If consistency is compromised by the loading order of plugins, then ConVat and ConCommand conflicts should not be allowed from the start. And even if we want to detect changes from the console, we should add a dedicated feature to ConVar, and not allow conflicts with ConCommand.
Your results are expected behaviours. When you load test1, test_command is a ConVar:
sp plugin load test1
[SP] Loading plugin 'test1'...
[SP] Successfully loaded plugin 'test1'.
test_command
"test_command" = "this is the value of test_command"Then, when you load test2, test_command becomes a ServerCommand proxy to the ConVar of test1:
sp plugin load test2
[SP] Loading plugin 'test2'...
[SP] Successfully loaded plugin 'test2'.
test_command
ConCommand test_commandWhatever you pass to that proxy gets assigned to the proxied ConVar:
test_command this is the new value of test_command
ConCommand test_commandWhen you unload test2, the proxy gets unregistered and test_command becomes a ConVar again:
sp plugin unload test2
[SP] Unloading plugin 'test2'...
[SP] Successfully unloaded plugin 'test2'.
test_command
"test_command" = "this is the new value of test_command" ( def. "this is the value of test_command" )Then when you unload test1, test_command is no more because the plugin that owns it is no more:
sp plugin unload test1
[SP] Unloading plugin 'test1'...
[SP] Successfully unloaded plugin 'test1'.
test_command
Unknown command "test_command"Now when you load them the other way around, you get an assertion error because the ConVar returned by:
assert cvar.find_var('test_command') is not None
Is unregistered because test_command is an existing ServerCommand. You can verify it via test_command.is_registered() which will returns False. From there you can insert it yourself in the registry, or not, etc.
If we were to raise instead of proxying existing or returning unregisered ConVar, we would get exceptions one way or another regardless.
Second, why did you make it possible to delete ConVar? This makes it very easy to cause a crash.
Because custom ConVar created by plugins should be released when said plugins are unloaded. Otherwise, they are just leaking forever. Just like anything else, if you start storing pointer wrappers you don't own, you will be in trouble if the owner decides he is done with them. Though, we could certainly make sure the same managed objects are returned and shared between plugins in such cases and kept alive until everyone is done with them.
CookStar
commented
2 years ago I understand your intentions, but I have a question, why?
Your results are expected behaviours
I am not asking how it behaves, I am asking why you are trying to introduce this behavior.
This is no different than you advocating plugin authors to write this kind of code when creating every ConVar.
from cvars import ConVar
from cvars import cvar
base_command = cvar.find_base('test_command')
if base_command is not None and base_command.is_command():
cvar.unregister_base(base_command)
test_command = ConVar('test_command', 'this is the value of test_command')get_server_command/ServerCommand and ConVar should always conflict so that one of them is prevented from being created. And, the user should be informed of the conflict as soon as possible, and if the plugin author/user wants to resolve the conflict, they can use cvar.unregister_base to do so.
Because custom
ConVarcreated by plugins should be released when said plugins are unloaded. Otherwise, they are just leaking forever. Just like anything else, if you start storing pointer wrappers you don't own, you will be in trouble if the owner decides he is done with them. Though, we could certainly make sure the same managed objects are returned and shared between plugins in such cases and kept alive until everyone is done with them.
ConVar must not be deleted under any circumstances. It should always leak. Just because it is a custom ConVar made from a Source.Python plugin, there is no guarantee that it will not be used by other valve plugins. Depending on the loading order of plugins, there is a good chance that ConVar will be owned by a plugin that should not own it. Hence, we never deleted ConVar in either Source.Python, Metamod, or SourceMod, right?
ConVar is not intended to be removed by any implementation. This includes Source.Python. This is a compatibility break. Even if it is not registered, ConVar must be alive until the end of srcds.
jordanbriere
commented
2 years ago ConVar must not be deleted under any circumstances. It should always leak. Just because it is a custom ConVar made from a Source.Python plugin, there is no guarantee that it will not be used by other valve plugins. Depending on the loading order of plugins, there is a good chance that ConVar will be owned by a plugin that should not own it. Hence, we never deleted ConVar in either Source.Python, Metamod, or SourceMod, right?
ConVar is not intended to be removed by any implementation. This includes Source.Python. This is a compatibility break. Even if it is not registered, ConVar must be alive until the end of srcds.
We are responsible to release the memory we use, just like they are for what they allocate. Nobody will do it for us, and they are also freeing theirs as can be shown with a simple hook:
from memory import *
from memory.hooks import *
from cvars import *
from commands import *
@PreHook(get_virtual_function(cvar, 'UnregisterConCommand'))
def pre_unregister_concommand(stack_data):
base = make_object(ConCommandBase, stack_data[1])
print(f'>> g_pCvar->UnregisterConCommand({"ConCommand" if base.is_command() else "ConVar"}("{base.name}"))')sp plugin load testing
[SP] Loading plugin 'testing'...
Hooking function: type=PRE, addr=1561814768, conv=THISCALL, args=(_memory.DataType.POINTER, _memory.DataType.POINTER), rtype=VOID, callback=<function pre_unregister_concommand at 0x209CC228>
[SP] Successfully loaded plugin 'testing'.
meta list
Listing 3 plugins:
[01] SourceMod (1.10.0.6502) by AlliedModders LLC
[02] CS Tools (1.10.0.6502) by AlliedModders LLC
[03] SDK Tools (1.10.0.6502) by AlliedModders LLC
meta unload 1
>> g_pCvar->UnregisterConCommand(ConCommand("sm_help"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_searchcmd"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_admin"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_abortban"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_addban"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_ban"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_banip"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_unban"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_chat"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_csay"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_hsay"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_msay"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_psay"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_say"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_tsay"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_gag"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_mute"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_silence"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_ungag"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_unmute"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_unsilence"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_cancelvote"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_cvar"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_execcfg"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_kick"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_map"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_rcon"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_reloadadmins"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_resetcvar"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_revote"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_who"))
>> g_pCvar->UnregisterConCommand(ConCommand("ff"))
>> g_pCvar->UnregisterConCommand(ConCommand("motd"))
>> g_pCvar->UnregisterConCommand(ConCommand("nextmap"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_vote"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_voteban"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_votekick"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_votemap"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_cookies"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_settings"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_beacon"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_blind"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_burn"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_drug"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_firebomb"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_freeze"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_freezebomb"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_gravity"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_noclip"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_timebomb"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_votealltalk"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_voteburn"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_voteff"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_votegravity"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_voteslay"))
>> g_pCvar->UnregisterConCommand(ConCommand("listmaps"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_maphistory"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_rename"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_slap"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_slay"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_play"))
>> g_pCvar->UnregisterConCommand(ConVar(""))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_dump_datamaps"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_dump_classes"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_dump_netprops"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_dump_netprops_xml"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_dump_teprops"))
>> g_pCvar->UnregisterConCommand(ConCommand("sm_print_telist"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_flood_time"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_chat_mode"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_deadtalk"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_trigger_show"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_timeleft_interval"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_vote_map"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_vote_kick"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_vote_ban"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_beacon_radius"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_timebomb_ticks"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_timebomb_radius"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_timebomb_mode"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_burn_duration"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_firebomb_ticks"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_firebomb_radius"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_firebomb_mode"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_freeze_duration"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_freezebomb_ticks"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_freezebomb_radius"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_freezebomb_mode"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_vote_gravity"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_vote_burn"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_vote_slay"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_vote_alltalk"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_vote_ff"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_reserved_slots"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_hide_slots"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_reserve_type"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_reserve_maxadmins"))
>> g_pCvar->UnregisterConCommand(ConVar("sm_reserve_kicktype"))
Plugin 1 unloaded.get_server_command/ServerCommand and ConVar should always conflict so that one of them is prevented from being created. And, the user should be informed of the conflict as soon as possible, and if the plugin author/user wants to resolve the conflict, they can use cvar.unregister_base to do so.
Alright, I thought more about it and I still can't find any valid reason why we should raise. To illustrate my thought process, let's suppose you have the following plugins:
# ../test1/test1.py
from cvars import cvar
from cvars import ConVar
test_command = ConVar('test_command', 'this is the value of test_command')# ../test2/test2.py
from commands.server import ServerCommand
from cvars import cvar
@ServerCommand('test_command')
def test_command(*args):
print("ConCommand test_command")Current implementation:
sp plugin load test1;sp plugin load test2
[SP] Loading plugin 'test1'...
[SP] Successfully loaded plugin 'test1'.
[SP] Loading plugin 'test2'...
[SP] Successfully loaded plugin 'test2'.sp plugin load test2;sp plugin load test1
[SP] Loading plugin 'test2'...
[SP] Successfully loaded plugin 'test2'.
[SP] Loading plugin 'test1'...
[SP] Successfully loaded plugin 'test1'.Raising:
sp plugin load test1;sp plugin load test2
[SP] Loading plugin 'test1'...
[SP] Successfully loaded plugin 'test1'.
[SP] Loading plugin 'test2'...[SP] Caught an Exception:
Traceback (most recent call last):
File "..\addons\source-python\packages\source-python\plugins\manager.py", line 207, in load
plugin._load()
File "..\addons\source-python\packages\source-python\plugins\instance.py", line 74, in _load
self.module = import_module(self.import_name)
File "..\addons\source-python\plugins\test2\test2.py", line 4, in
sp plugin load test2;sp plugin load test1 [SP] Loading plugin 'test2'... [SP] Successfully loaded plugin 'test2'. [SP] Loading plugin 'test1'...
[SP] Caught an Exception:
Traceback (most recent call last):
File "..\addons\source-python\packages\source-python\plugins\command.py", line 164, in load_plugin
plugin = self.manager.load(plugin_name)
File "..\addons\source-python\packages\source-python\plugins\manager.py", line 207, in load
plugin._load()
File "..\addons\source-python\packages\source-python\plugins\instance.py", line 74, in _load
self.module = import_module(self.import_name)
File "..\addons\source-python\plugins\test1\test1.py", line 4, in
ValueError: ConVar "test_command" already exists.
I personally don't see the reason for preventing one or the other to load entirely because this isn't a critical issue whatsoever. Worst case scenario of the former is that `test1`'s `ConVar` won't be assignable from the console if `test2` was loaded first, but the plugin itself will still work normally and will be using its default value when querying its `ConVar` which in itself, as stated, isn't critical. As for the following:
> I am not asking how it behaves, I am asking why you are trying to introduce this behavior.
>
> This is no different than you advocating plugin authors to write this kind of code when creating every ConVar.
>
> ```python
> from cvars import ConVar
> from cvars import cvar
>
> base_command = cvar.find_base('test_command')
>
> if base_command is not None and base_command.is_command():
> cvar.unregister_base(base_command)
>
> test_command = ConVar('test_command', 'this is the value of test_command')
> ```
>
> get_server_command/ServerCommand and ConVar should always conflict so that one of them is prevented from being created. And, the user should be informed of the conflict as soon as possible, and if the plugin author/user wants to resolve the conflict, they can use cvar.unregister_base to do so.
No, definitely not. And such implementation would be highly discouraged. Plugin developers should not care whether their `ConVar` are linked to the console and should simply register them normally. This is a server administrators issue if they are not assignable, not a plugin developers issue. The only thing that would make sense from my point of view, would be to let the server administrators aware. For example, with a warning:
sp plugin load test2;sp plugin load test1 [SP] Loading plugin 'test2'... [SP] Successfully loaded plugin 'test2'. [SP] Loading plugin 'test1'... [SP] ConVar "test_command" won't be assignable from the console, or any CFG file, because a ConCommand with the same name is already registered. [SP] Successfully loaded plugin 'test1'.
Other than that, yeah, I really cannot justify to prevent one or the other to load in such case. I tried to find scenarios where it would make sense, but I was unable to. Though, perhaps I'm entirely mistaken. I would definitely like for others to chime in and give their opinion on this.We are responsible to release the memory we use, just like they are for what they allocate. Nobody will do it for us,
No, we can't be responsible, and no one can be responsible except srcds.
and they are also freeing theirs as can be shown with a simple hook:
First of all, UnregisterConCommand does not delete/free a ConVar/ConCommand. Secondly, UnregisterConCommand has nothing to do with this problem.
I will repeat myself.
Just because it is a custom ConVar made from a Source.Python plugin, there is no guarantee that it will not be used by other valve plugins.
Just because the Source.Python plugin created ConVar, it doesn't mean anything. It's just that the Source.Python plugin was the first to create the ConVar.
Intentional or unintentional, either way, ConVar can overlap. And since SourcePython's ConVar(str) and SourceMod's CreateConVar returns an existing ConVar, it is not possible to delete ConVar.
CookStar
commented
2 years ago Other than that, yeah, I really cannot justify to prevent one or the other to load in such case. I tried to find scenarios where it would make sense, but I was unable to. Though, perhaps I'm entirely mistaken. I would definitely like for others to chime in and give their opinion on this.
I think you are not considering the case where the conflict between ConVar and ConCommand is unintentional, i.e. by accident. If conflicts occur due to this accident, it is difficult to reproduce and debug the situation. This is because there may be multiple authors implementing each plugin for ConVar and ConCommand.
Worst case scenario of the former is that
test1'sConVarwon't be assignable from the console iftest2was loaded first, but the plugin itself will still work normally and will be using its default value when querying itsConVarwhich in itself, as stated, isn't critical.
That's because you have a good understanding of the ConVar and ConCommand specifications, but there's no guarantee that the plugin authors understand them, right? Once a ConVar has been created, it should be retrievable by cvar.find_var, since other plugins may refer to the conflicted ConVar.
For example, suppose you have a plugin that accidentally has a conflict, and you have a wrapper plugin for ConVar:
Plugin 1:
test_command = ConVar('test_command', 'this is the value of test_command')Plugin 2:
@ServerCommand('test_command')
def test_command(*args):
print("ConCommand, which has been overridden by accident.")Plugin 3 (Wrapper Plugin):
test_command = cvar.find_var('test_command')
assert test_command is not NoneThis will fail.
Edited: And what about ConCommand, which is not a CServerCommandManager?
Plugin test1:
sm = ConVar('sm', 'A ConVar that is not intended to be overridden by a ConCommand.')Plugin test2:
@ServerCommand('sm')
def sm_command(*args):
print("ConCommand sm")Output:
sp plugin load test1
[SP] Loading plugin 'test1'...
[SP] Successfully loaded plugin 'test1'.
sm
"sm" = "A ConVar that is not intended to be overridden by a ConCommand."
sp plugin load test2
[SP] Loading plugin 'test2'...
[SP] Successfully loaded plugin 'test2'.
sm
ConCommand sm
meta load addons/sourcemod/bin/sourcemod_mm
Plugin "SourceMod" loaded with id 1.
sm
ConCommand sm
sp plugin unload test2
[SP] Unloading plugin 'test2'...
[SP] Successfully unloaded plugin 'test2'.
sm
"sm" = "" ( def. "A ConVar that is not intended to be overridden by a ConCommand." )
sp plugin unload test1
[SP] Unloading plugin 'test1'...
[SP] Successfully unloaded plugin 'test1'.
sm
Unknown command "sm"This is an unintended result because the plugin can be loaded when it should fail to load. I used sm for the example, but any ConCommand created by outside of SourcePython will have this problem. Edited End.
There is no end to such edge cases that are not kind to plugin authors. And to avoid such problems, conflicts between ConVar and ConCommand should be prohibited in principle, as in SourceMod. And if the plugin author wants to intentionally introduce conflicts, i.e. override ConVar with ServerCommand, that is up to the plugin author.
Finally, and I'll ask this again, why are you trying to introduce this behavior? It seems like an unnecessary and confusing behavior to me.
jordanbriere
commented
2 years ago First of all, UnregisterConCommand does not delete/free a ConVar/ConCommand. Secondly, UnregisterConCommand has nothing to do with this problem.
They are being unregistered, because they are being released. For example:
# ../testing/testing.py
from commands.server import ServerCommand
from cvars import ConVar
sm_flood_time = ConVar('sm_flood_time')
@ServerCommand('testing')
def testing(command):
sm_flood_time.set_float(1.0)Then run sp plugin load testing;meta unload 1;testing and watch yourself crash because the pointer wrapped by sm_flood_time has been released. If you are still not convinced, go ahead and hook their destructor:
@PreHook(get_virtual_function(cvar, 'UnregisterConCommand'))
def pre_unregister_concommand(stack_data):
base = make_object(ConCommandBase, stack_data[1])
print(f'>> g_pCvar->UnregisterConCommand({"ConCommand" if base.is_command() else "ConVar"}("{base.name}"))')
sm_flood_time = ConVar('sm_flood_time')
@PreHook(get_object_pointer(sm_flood_time).make_virtual_function(0, Convention.THISCALL, (DataType.POINTER,), DataType.VOID))
def pre_convar_destructor(stack_data):
print(f'>> ConVar::~ConVar("{make_object(ConVar, stack_data[0]).name}")')
return FalseYou will crash on return due to lack of return type which is not supported by DynamicHooks, but you will have plenty of time to observe the console:
sp plugin load testing;meta unload 1
>> g_pCvar->UnregisterConCommand(ConVar("sm_flood_time"))
>> ConVar::~ConVar("sm_flood_time")
>> g_pCvar->UnregisterConCommand(ConVar("sm_chat_mode"))
>> ConVar::~ConVar("sm_chat_mode")
>> g_pCvar->UnregisterConCommand(ConVar("sm_deadtalk"))
>> ConVar::~ConVar("sm_deadtalk")
...Just because the Source.Python plugin created ConVar, it doesn't mean anything.
It actually means everything. It means SP owns its memory, and is responsible to release it. Period. The fact it may or may not be used externally is simply irrelevant and not our responsibility whatsoever. What is our responsibility, however, is to make sure we handle memory we ourselves borrow externally accordingly. Which, we probably should, but is an entirely different topic.
Intentional or unintentional, either way, ConVar can overlap. And since SourcePython's ConVar(str) and SourceMod's CreateConVar returns an existing ConVar, it is not possible to delete ConVar.
We are only releasing the ones we allocated, not existing ones. Everyone can tell whether they own a specific one or not. First, because well, they are the one new'ing it, second, they can compare their ID with theirs at any given time. In any case, purposely leaking memory we allocate is simply not the solution.
For example, suppose you have a plugin that accidentally has a conflict, and you have a wrapper plugin for ConVar:
Plugin 1:
test_command = ConVar('test_command', 'this is the value of test_command')Plugin 2:
@ServerCommand('test_command') def test_command(*args): print("ConCommand, which has been overridden by accident.")Plugin 3 (Wrapper Plugin):
test_command = cvar.find_var('test_command') assert test_command is not NoneThis will fail.
Hmm. Did you even test? It feel like you based this example on assumptions, because well, It won't fail. Into Plugin 3, cvar.find_var will return Plugin 1's ConVar and cvar.find_command will return Plugin 2's command.
This is an unintended result because the plugin can be loaded when it should fail to load. I used sm for the example, but any ConCommand created by outside of SourcePython will have this problem.
I'm not entirely sure what you are trying to point out here, but I assume that last line?
sm
Unknown command "sm"If so, then it has nothing to do with us. Even without SP involved, if you load SM that way, the sm command won't be registered. As you can see there:
plugin_print
Loaded plugins:
---------------------
0: "Metamod:Source 1.11.0-dev+1145"
---------------------
meta list
No plugins loaded.
meta load addons/sourcemod/bin/sourcemod_mm
Plugin "SourceMod" loaded with id 1.
meta list
Listing 1 plugin:
[01] SourceMod (1.10.0.6502) by AlliedModders LLC
sm
Unknown command "sm"I have no idea why, nor do I care honestly, but yeah, it have nothing to do with us whatsoever.
There is no end to such edge cases that are not kind to plugin authors. And to avoid such problems, conflicts between ConVar and ConCommand should be prohibited in principle, as in SourceMod. And if the plugin author wants to intentionally introduce conflicts, i.e. override ConVar with ServerCommand, that is up to the plugin author.
There will always be clashes involved whenever we try to eat into each other's plate. This is not new. At the end of the day, this is the responsibility of server owners to make sure the plugins they install are compatible with each others. Sure, we are making effort. However, trying to predict what others may or may not do would never end. I personally didn't find your examples to be very convincing, and I still hold the same position on the topic as I did in my previous post. Just like everything else, if you dig long enough, you will find something. Perhaps even a treasure, who knows.
Finally, and I'll ask this again, why are you trying to introduce this behavior? It seems like an unnecessary and confusing behavior to me.
Because as previously stated, I don't see any valid reason to prevent plugins to load in such cases. To me, raising exceptions "is" what would introduce conflicting situations.
CookStar
commented
2 years ago I created a new issue #430 because this problem is not related to this pull request. I will also create an issue about ConCommand and ConVar conflicts, but that will come later.
They are being unregistered, because they are being released.
The reason ConVar is being released is because SourceMod is releasing it intentionally. As I've said many times before, this problem has nothing to do with UnregisterConCommand. https://github.com/alliedmodders/sourcemod/blob/6a2ac9800bfd10ec986d18d2286177f4cbc52803/core/ConVarManager.cpp#L132-L141
Then run sp plugin load testing;meta unload 1;testing and watch yourself crash because the pointer wrapped by sm_flood_time has been released.
First of all, unloading VSP/Meta Plugins and unloading SourcePython plugins should not be discussed in the same way. Secondly, SourceMod is releasing ConVar because it does not acknowledge the existence of VPS plugins like Source.Python. Thirdly, at least SourceMod releases ConVar when unloading VPS/Meta Plugins, not when unloading Source.Python plugins as you have implemented.
And this is bad design and should not be done.
Just because the Source.Python plugin created ConVar, it doesn't mean anything.
It actually means everything. It means SP owns its memory, and is responsible to release it. Period. The fact it may or may not be used externally is simply irrelevant and not our responsibility whatsoever. What is our responsibility, however, is to make sure we handle memory we ourselves borrow externally accordingly. Which, we probably should, but is an entirely different topic.
Intentional or unintentional, either way, ConVar can overlap. And since SourcePython's ConVar(str) and SourceMod's CreateConVar returns an existing ConVar, it is not possible to delete ConVar.
We are only releasing the ones we allocated, not existing ones. Everyone can tell whether they own a specific one or not. First, because well, they are the one
new'ing it, second, they can compare their ID with theirs at any given time. In any case, purposely leaking memory we allocate is simply not the solution.
You are not understanding the root of the problem, and you are missing the perspective from the plugin side. If your stance is that it's not Source.Python's responsibility for other VSPs (especially SourceMod) to use ConVars generated by Source.Python, that's fine, but it's wrong that Source.Python plugins can't own each other's ConVars, right? As long as ConVar(str) returns an existing ConVar, how can a plugin be sure it is safe to own it? This means that all plugins must always check for ConVar and unregister it if it exists, to safely generate it.
Code:
from cvars import ConVar
from cvars import cvar
var = cvar.find_var("test_convar")
if var is not None:
cvar.unregister_base(var)
test_convar = ConVar("test_convar", "test", "Test Convar.")I created a new issue #430 because this problem is not related to this pull request. I will also create an issue about ConCommand and ConVar conflicts, but that will come later.
We should rather keep all the related discussions into the same thread. Multiple issues/PRs will only promote cross-referencing each others as well as duplicated posts iterating the same points and whatnot.
The reason ConVar is being released is because SourceMod is releasing it intentionally. As I've said many times before, this problem has nothing to do with UnregisterConCommand. https://github.com/alliedmodders/sourcemod/blob/6a2ac9800bfd10ec986d18d2286177f4cbc52803/core/ConVarManager.cpp#L132-L141
You know, if you re-read the thread, you will realize that the only reason why I mentioned a hook on UnregisterConCommand was to show yourself that your initial claim that MM/SM never ever release their ConVar was not accurate:
Hence, we never deleted ConVar in either Source.Python, Metamod, or SourceMod, right?
Then, you counter-argued my example stating that, although they are being unregistered, it doesn't mean they are freed. Which, fair enough, so I provided a hook on their destructor to further show you that they are being unregistered because they are being released. Now, you backtrack saying they are being released but still argue the examples I provided to debunk your original claim so I'm a bit confused as to whether I'm completely misinterpreting what you are trying to say.
Secondly, SourceMod is releasing ConVar because it does not acknowledge the existence of VPS plugi
And they should not have to. As previously mentioned, this is their responsibility to handle memory they don't own accordingly just like it is our to do so:
The fact it may or may not be used externally is simply irrelevant and not our responsibility whatsoever. What is our responsibility, however, is to make sure we handle memory we ourselves borrow externally accordingly. Which, we probably should, but is an entirely different topic.
In fact, SM is listening for unregistrations and is internally unlinking their references so that they don't manipulate unregistered instances they don't own. We should probably implement something similar, but again, purposely leaking memory will never be the solution in my book.
but it's wrong that Source.Python plugins can't own each other's ConVars, right?
I understand the issue perfectly. I even acknowledged it and stated that we should address it if you scroll up a few posts:
Though, we could certainly make sure the same managed objects are returned and shared between plugins in such cases and kept alive until everyone is done with them.
In any case, any issues you can possibly bring up regarding ConVar also apply to our commands because we free them as soon as plugins are done with them regardless if there is still references floating around. Also, any conflict between us and MM/SM is our fault and our responsibility because we handle their memory as if we own it but we don't.
To be fair, I don't mind whatever. I said everything I had to say, and made my position on the topic very clear so I'm not sure what else can be said from my end. You guys can go about it whatever way you agree on.
There is already
OnConVarChanged, but it is wasteful if you only want to know the changes of a specific ConVar. This was made to match SourceMod's AddChangeHook feature. https://wiki.alliedmods.net/ConVars_(SourceMod_Scripting)#Change_CallbacksCode:
Output: