Pause dependent plugins on SetFailState (bug 6120) #93
Conversation
| @@ -376,7 +376,7 @@ static cell_t SetFailState(IPluginContext *pContext, const cell_t *params) | |||
|
|
|||
| if (params[0] == 1) | |||
| { | |||
| pPlugin->SetErrorState(Plugin_Error, "%s", str); | |||
| pPlugin->SetErrorState(Plugin_Failed, "%s", str); | |||
There was a problem hiding this comment.
These should stay as Plugin_Error, I think... if it's Plugin_Failed, we assume the error was so severe that we can't perform diagnostics (like say, the underlying plugin structure may be read as corrupt, so sm plugins info <N> will display a very abbreviated listing). Since that's not the case here, "Error" should be good enough.
There was a problem hiding this comment.
I see, i tried to avoid changing the signature of CPlugin::SetErrorState and still be able to DropEverything only when the plugin is not going to resume.
I could live with that drawback of less info in sm plugins info.
There was a problem hiding this comment.
Hrm, I don't understand that comment.
There was a problem hiding this comment.
The whole point of this PR is to call DropEverything on the plugin which is paused by the SetFailState native.
It does this by passing the Plugin_Failed status to CPlugin::SetErrorState, which was modified to drop everything, if called with Plugin_Failed.
SetErrorState also calls CPlugin::SetPauseState now instead of only CPluginManager::_SetPauseState, so the libraries of the plugins are updated with the OnLibrary(Added|Removed) forwards (and OnPluginPauseChange is called).
We could still change the signature of SetErrorState to include another boolean which controls calling of DropEveryhing to pause dependent plugins, but i tried to avoid changing the intercom system.
There was a problem hiding this comment.
Bleh... the status code stuff is such a mess. Okay, if we go this route then, it would be good to change:
https://github.com/alliedmodders/sourcemod/blob/master/core/logic/PluginSys.cpp#L1141
That to Plugin_BadLoad, and..
https://github.com/alliedmodders/sourcemod/blob/master/core/logic/PluginSys.cpp#L2220
https://github.com/alliedmodders/sourcemod/blob/master/core/logic/PluginSys.cpp#L2243
These two include Plugin_Failed.
This way the meaning of "Failed" becomes "An error that is unrecoverable" and the most severe existing uses of "Failed" become "unloadable plugin."
|
The rest looks fine, though I'm guessing we could just remove OnPluginPauseChange (do we even support pausing anymore?) |
|
Internally plugins still get paused by the dependency system. You can't do sm plugins pause x anymore though, so the user can't control pausing. I yet have to see a plugin that handles pausing itself with that forward, but if there are still cases that would have this called, i'd just leave it in. |
|
That actually sounds like a pretty good argument for removing the callback, not keeping it ;) but since it's unrelated to this bug it's fine to keep it as-is. |
|
Changed the usage of Plugin_Failed as you suggested. |
|
@peace-maker I think this has needed a rebase - should be good to go otherwise though. |
When a plugin calls SetFailState it is paused and all natives it registered are unavailable. Other plugins, which depend on those natives keep running and error whenever they try to call those natives. This correctly sets the dependent plugins to an error state as if the plugin which called SetFailState was unloaded.
Change the meaning of Plugin_Failed status to indicate, that the plugin can't recover from the error. Make sure those previously loaded plugins are shown correctly in sm plugins info x.
peace-maker
force-pushed
the
failstate_dependencies
branch
from
7abd3a9
to
9b2e777
Compare
|
Good to go |
Pause dependent plugins on SetFailState. (bug 6120, r=dvander)
When a plugin calls SetFailState it is paused and all natives it
registered are unavailable. Other plugins, which depend on those natives
keep running and error whenever they try to call those natives.
This correctly sets the dependent plugins to an error state and also calls OnLibraryRemoved correctly for all libraries as if the
plugin which called SetFailState was unloaded.
As a sideeffect OnPluginPauseChange is called again, when a plugin changes its pause state.