Open fm-117 opened 1 week ago
LoggingSystem
is also part of the 'problem', we should improve the current implementation by using an AutoResetEvent
instead of a periodic poll.
LanguageServer.NotifyWarning
and LanguageServer.NotifyException
are the methods dedicated to signal a "Warning" or an exception to the client
NotifyWarning
is almost irrelevant as it is used only in TypeCobol context to warn about problems during intrinsics or dependencies loading. This is not used in pure-Cobol context.NotifyException
is the main target of this issue, the goal is to ensure a telemetry/event
is sent to the client when it is calledNotifyException
is used in many contexts:
Call
or Received
) uses the same pattern and have a try-catch
with a call to NotifyException
in the catch
block.JsonRPCServer
methods: HandleNotification
, HandleRequest
and HandleResponse
also have a catch
block calling indirectly NotifyException
. This is redundant with the pattern mentioned above.NotifyException
is called on the AppDomain.UnhandledException
eventNotifyException
is called when the semantic update timer thread crashesNotifyException
is called by TypeCobolServerHost.MessageHandler
to protect agains exception happening when executing an auto-message (Action
instance created by the server to perform work on its own message queue).We should cover all threads working simultaneously in the server to ensure the application won't crash and also track all exceptions. In our LanguageServer, we have at most 5 concurrent threads:
didChange
event receivedLoggingSystem
, it is responsible for performing logging actionsFollowing table states how each thread is protected against crash:
Thread | Status | TODO |
---|---|---|
main | not protected directly, but a crash will be caught by UnhandledException event |
Check if some exceptions currently caught can be notified |
worker | All client messages are covered by 2 catch blocks, each having a notify mechanism, auto-messages are covered too | Simplify code ? Avoid redundant catch blocks ? |
background compilation | Covered by a can block + Notify | |
logger | Not covered but it's ok, logging is secondary and should be considered as safe | Improve implementation using AutoResetEvent |
message sender | Not covered | The ThreadBody does not have its own try-catch block, waiting the sending task is alsso unsafe |
Unlike other locations, AppDomain.UnhandledException
denotes a fatal problem: a window/showMessage
should be sent to the client to signal the server has crashed.
Proposal for event serialization:
public class TelemetryEvent
{
public string type;
public Dictionary<string, string> data;
}
string
field to define the type of the event and help the observer (client) to deserialize the associated dataFor example, for an exception, data
would contain:
"Type": "System.ArgumentException"
"Message": "Invalid argument for param 'example'"
"TargetSite": "TypeCobol.Compiler.SomeClass.SomeMethod(ExampleType)"
To improve monitoring of our LanguageServer, we have to introduce some telemetry events for server exceptions. This issues encompasses multiple goals :
telemetry/event
LSP methodwindow/showMessage
opposite totelemetry/event