Improve command error handling with compatibility detection and early validation - #115
Conversation
|
Honestly Carbon builds should capture which protocol number and staging-or-not flavor of RustDedicated they're intended for, and warn/annoy the hell out of the server owner if there's a mismatch. Carbon almost always needs a new build for protocol bumps (exceptions like the month deep sea got delayed are rare), so we shouldn't be optimizing for that by having Carbon ignore likely version mismatches. |
That's a good suggestion. The architecture for that behavior already partially exists for the auto updater, so that would have to be a seperate PR. Also maybe I'm misunderstanding but your comment implies that the PR makes Carbon less likely to catch version mismatches. It doesn't, it's the opposite actually. Examples why this reactive strategy is useful:
Thanks again for the feedback, let me know if anything is unclear @HunterZ |
What the PR is doing and what I was saying are both trying to address Carbon-RustDedicated mismatches, but in different ways. This PR is more focused on the pointy end, and I was just wondering if there's value in trying to catch it earlier and/or more definitively. Concerns with this PR:
Arguments in favor of this PR:
|
|
These approaches aren't mutually exclusive. A startup check wont catch runtime mismatches from hot reloaded plugins or function level sig changes. Feel free to request me as a PR reviewer if you wanna persue that! |
|
Doesn't the existing message already tell you the bad call in the plugin though? Are we just swallowing all the exceptions now so plugins can kind of work around incompatible functionality? I'm a bit confused byt he nature of this. The summary in the PR says it's just adding better logging essentially, but the actual code looks like it's adding a bunch of tries, catching exceptions, and potentially changing behavior entirely? Maybe I am reading it wrong? |
|
All exceptions are still logged via Logger.Error; the only change is that compatibility errors now get a clearer message and an optional, OFF by default notification. |
713113e to
d8a874e
Compare
d0b6dcf to
9502e7b
Compare
- Refactor ExceptionEx.IsCompatibilityMissingMember to parse the missing member from MissingMethodException/MissingFieldException messages using regex, then resolve the declaring type via AccessToolsEx.TypeByName and fall back to a compatibility assembly type scan. - Deduplicate missing member extraction into TryExtractMissingMember and reuse it in ExtractMissingMember. - Update Command.AddConsoleCommand RCon callback to set args.Reply to the exception message when PrintOutput is false, so compatibility and generic callback errors are returned to the caller instead of being swallowed.
EthanDelong
left a comment
There was a problem hiding this comment.
The main concerns I have are around the cost of the Regex/assembly search in a Mono process. Every time someone executes a command that hits the incompatible point, it looks like it allocates. The fallback assembly/type scan is uncached and the detection re-runs several times per exception, so this possibly introduces a way for users to flood a server without meaning to. A plugin might be running "fine" with broken commands and owners just don't know or care about that specific usage.
Some of the console callbacks throw in existing paths. I understand the chat commands already have the try/catch, but console did not. Those exceptions previously propagated up to CommandManager.Execute, which returns false on failure and forwards the full exception to RCON. With the catch moved inside the callback, Execute now returns true for a command that threw, and the RCON output changes. That might be fine, but it's a behavior change and I think the summary should mention it. Also worth noting the compatibility filter runs first on every failed command, before the generic catch.
I'd also pull the unrelated cleanup (the ?? [] changes) into a separate PR just to keep this one clean.
That said, the HookEx change is a nice improvement. That alone I think is worth keeping. I am just not certain about the extra walks just for a friendlier exception message upstream. If it was limited to HookEx changes I would say it's perfect.
| arg.Option = client; | ||
| arg.FullString = fullString; | ||
| arg.Args = [.. args.Select(x => (StringView)x)]; | ||
| arg.Args = [.. (args ?? []).Select(x => (StringView)x)]; |
There was a problem hiding this comment.
cleanup unrelated to PR context
| try { callback?.Invoke(playerArgs.Player as BasePlayer, command, arg.Arguments.ToStringArray()); } | ||
| catch (Exception ex) when (ex.IsCompatibilityError()) { LogCommandCompatibilityError("console", command, plugin, ex, "callback", playerArgs.Player as BasePlayer, isChat: false); } | ||
| catch (Exception ex) { LogCommandGenericError("console", command, plugin, ex, "callback"); } | ||
| break; |
There was a problem hiding this comment.
existing case that used to throw before, will now be caught and produce a specific ex without re-throwing. have we tested this specific and what the before/after is? does it align with the PR's request which is specifically for producing compatibility messages when it suspects them?
| callback?.Invoke(null, command, arg.Arguments.ToStringArray()); | ||
| try { callback?.Invoke(null, command, args.Arguments.ToStringArray()); } | ||
| catch (Exception ex) when (ex.IsCompatibilityError()) { LogCommandCompatibilityError("console", command, plugin, ex, "callback", null, isChat: false, notifyPlayer: false); if (!args.PrintOutput) args.Reply = ex.Message; } | ||
| catch (Exception ex) { LogCommandGenericError("console", command, plugin, ex, "callback"); if (!args.PrintOutput) args.Reply = ex.Message; } |
There was a problem hiding this comment.
same as before, swallowing exception previously would've been thrown
| arg.Option = option; | ||
| arg.FullString = fullString; | ||
| arg.Args = args.ToStringViewArray(); | ||
| arg.Args = (args ?? []).ToStringViewArray(); |
There was a problem hiding this comment.
unrelated cleanup again
| args.PrintOutput = arg.Option.PrintOutput; | ||
| } | ||
| catch (Exception ex) when (ex.IsCompatibilityError()) { LogCommandCompatibilityError("console", command, plugin, ex, "callback", arg.Player(), isChat: false); } | ||
| catch (Exception ex) { LogCommandGenericError("console", command, plugin, ex, "callback"); } |
| args.Reply = arg.Reply; | ||
| } | ||
| catch (Exception ex) when (ex.IsCompatibilityError()) { LogCommandCompatibilityError("console", command, plugin, ex, "callback", arg.Player(), isChat: false, notifyPlayer: false); if (!args.PrintOutput) args.Reply = ex.Message; } | ||
| catch (Exception ex) { LogCommandGenericError("console", command, plugin, ex, "callback"); if (!args.PrintOutput) args.Reply = ex.Message; } |
| return AppDomain.CurrentDomain.GetAssemblies() | ||
| .Where(assembly => IsCompatibilityAssembly(assembly.GetName().Name)) | ||
| .SelectMany(AccessToolsEx.GetTypesFromAssembly) | ||
| .Any(type => type.FullName != null && type.FullName.Replace('+', '.').Equals(declaringTypeName, StringComparison.Ordinal)); |
There was a problem hiding this comment.
this feels really expensive. if a plugin has an incompatible method, and a player spams it, what's the impact of running this over and over?
Summary
This PR detects version mismatch exceptions in plugin commands and hooks, producing clearer console output and optionally notifying players.
Console command callback exceptions are now handled within the registered command callback rather than propagating to CommandManager.Execute. As a result, Execute completes its normal path and returns true even when the underlying plugin callback fails. For RCON commands with output disabled, the callback places the error in args.Reply, allowing Execute to forward it through the normal RCON reply path instead of its outer exception handler. Generic failures retain the full exception and stack trace, while compatibility failures include both the friendly version-mismatch diagnosis and the resolved compatibility exception details.
Before (generic error for all command failures):
After (generic error, unchanged behavior):
If ShowCommandCompatibilityErrors is enabled in config, the player also sees:
Hook target class not found now fails early with: