Send events through a single stream

[aozgaa]

EDIT: also fixes #19665

We would like to send projectInfo events to VS. The stdin/stdout streams for communication between VS and tsserver are necessarily serialized as a parsing optimization in the client (VS). So, we cannot send events to VS through stdin/stdout.*

A natural alternative (implemented here) is to send the projectInfo event over the event port, which is currently used for typings installer events. To do this, the ProjectService instance needs to be able to issue events through the event port. ProjectService sends events through an eventHandler argument which is owned by the Session instance, so I modified the Session constructor to pick whether the instance will send events through stdin/stdout (as some editors, eg vscode, take events over stdout) or through the event port.

Unless a user of the Session API overwrites the eventHandler argument, all events will now be treated consistently.

---

These changes are backwards-compatible, and require no changes from clients. When running against a tsserver with these changes, I've validated that vscode and VS continue to receive and parse all the events they previously received. In vscode, all messages continue to be sent through stdin/stdout. In VS, I've checked that typingsInstaller events continue to be sent through the event port.**

* Note that vscode does not use this mechanism -- all messages, including events, are sent through stdin/stdout.

** More precisely, typingsInstaller events are sent via stdin/stdout from the typingsInstaller process to tsserver, where they are packaged into tsserver-protocol-compatible events and then sent to the client. The same mechanism is used in the status quo.

[sheetalkamat]

Whats the purpose of setting canUseEvents as true all the time?

[aozgaa]

With this change, we can send events regardless of whether eventPort is set or not. If eventPort === undefined (ie: sending events through stdin/stdout), the behavior is unchanged. But now tsserver can send events through the event port, so if eventPort !== undefined, we can send events as well.

[aozgaa]

I kept the canUseEvents option available to avoid breaking the public API. As far as I am aware, @anstarovoyt is the only direct consumer of the IoSession API. Perhaps he can comment on whether canUseEvents is a useful option for him. And if not, we can consider deprecating and eventually removing canUseEvents.

But we should do this in a separate issue.

[billti]

What types do you need for this? I see you are defining the 'net' module types inline below.

[aozgaa]

The reference is needed for require and Buffer to be defined, used to import "net" and write events to the socket, respectively.

[vladima]

my 2 cents: session.ts used to be a abstracted from any host specific bits so it can be used in any environment + it was also very convenient for testing since anything can be mocked. All host specific bits were injected from the outside in iosession.ts.

[billti]

I don't see any changes that are sending 'projectInfo' events. Is that not part of this pull request?

Also, as discussed, I'm not sure this is the right approach. Why isn't the projectInfo just sent back as part of the response to a request to open or update a project, and the editor can handle it then? This isn't really an 'event'. projectInfo doesn't just happen out of band or complete at some later time.

[aozgaa]

I'll answer these in reverse order (since the first question is moot if we'd rather take another approach).

Since the event is already used as a mechanism for reporting telemetry in vscode, replacing it with a property on a response would introduce two mechanisms we need to maintain: the (vscode) client needs to be able to process events to get projectInfo telemetry from users on tsserver versions not including new response properties. We could deprecate the event to eventually remove it.

---

Here are the reasons why projectInfo was originally added as an event, along with potential workarounds:

1. one concern when implementing this is that there wasn't a good response onto which we would add the projectInfo properties. However, I see that there is a ProjectInfo request that looks like a reasonable basis for a response.
2. If we send projectInfo data back as part of every, say, ProjectInfo response, then we would repeatedly be sending the same info back, and it would be wasteful to report the same info multiple times. there is a check to ensure the projectInfo event isn't issued multiple times, but I do not see why the same check cannot be done to avoid adding the telemetry data onto a response.
3. TelemetryEvent in the protocol provides a convenient mechanism for vscode to funnel along data from tsserver without adding custom-logic for each telemetry event. Adding projectInfo as an event compatible with this interface was an implementation convenience for the vscode use-case. (We could process all telemetry events in a similar way in VS with this proposed change in place). Note that there are really only two telemetry events right now -- TypingsInstalledTelemetryEventBody and the projectInfo telemetry.

---

The logic for sending the projectInfo event is unchanged inside editorServices.ts (the eventSender argument was added to plumb a mechanism for sending events to the typingsInstaller, which previously initialized the eventPort itself), but the changes to the Session effect how the projectInfo message is handled.

Note that in VS, eventPort !== undefined because the eventPort is used to send typingsInstaller events.

In editorServices.ts, the ProjectService instance issues a projectInfo event when a configured or external project is created.* In the status quo, the Session swallows this event because canUseEvents === false iff eventPort !== undefined . With this change, the Session now owns the eventPort, and so we can set canUseEvents === true when eventPort !== undefined, and actually send the event.

[sheetalkamat]

@aozgaa All points taken but why is it not that IOSession open the socket and give the information to typing installer and also set the eventHandler for ProjectServiceOptions that sends that event through the socket?

[aozgaa]

I tried exposing the defaultEventHandler as a static method, but it is tightly coupled to the implementation of other methods and state within Session, so it cannot be exposed as static without removing the dependency on the state of the projectService, which would require substantially changing the behavior of the defaultEventHandler.

This is to say, at the initialization of the Session instance from IOSession, we could inject a custom eventHandler, but we cannot expose the defaultEventHandler for extending without overhauling the API.

[aozgaa]

I've retained the initialization of the socket here for backwards compatibility with users initializing Session with the eventPort argument. This way, the typings installer will be able to send events through the socket by calling event on the Session.

[aozgaa]

Oops, realized that I actually added the eventPort argument in an earlier commit of this PR, so this is unnecessary. Will fix.

[aozgaa]

It is a bit awkward that this sender duplicates the implementation of Session.prototype.event.

If someone subclasses Session and overwrites send, the event implementation in Session will call the overridden send. This "default" sender will not.

Overall this feels like an imperfect way of factoring out the default behavior. Any suggestions for how to do this better?

[aozgaa]

Per review with @mhegazy , the problem that led to the extra complexity with the factored-out event-senders is that events are being sent before the constructor finishes. So, I'll instead defer any events that get fired while we are still in the (IO)Session constructor (ie: before the object owning send and event is initialized).

The simplest approach we came up with is to register a callback that fires on the next tick for issuing the offending events (namely, TypingsInstallerPID), and document this requirement.

[sheetalkamat]

Good way to add test would be to create a derived class of Sessions in the test (in src/harness/unittests/tsserverProjectSystem.ts) such that it has this.event overriden and verify that you receive events?
Changes look good otherwise