Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extension Host: Send initialization data #189

Merged
merged 22 commits into from
Mar 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/editor/Core/Filesystem.rei
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ let mkdir: (string, ~perm: int=?, unit) => t(unit);

let rmdir: string => t(unit);

let unsafeFindHome: unit => string;
Copy link
Member

@akinsho akinsho Mar 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bryphe unsafe find home can cause a runtime error, I hide it to prevent its usage an alternative which is safer is to use a wrapped version that returns an Error or Ok like the rest of Filesystem just a heads up 👍 . An alternative is to use getHomeDirectory which returns

type t('a) = {
 | Error(string),
 | Ok(a)
}

That way this function has less of a chance to fail at runtime and kill the app for a user

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't require any infix operators or anything if that's a concern

Copy link
Member Author

@bryphe bryphe Mar 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback and looking over the PR, @Akin909 !

What should we do in the case where we don't have a home directory (path for logs or storage) for the extension host client?

If we use the result object, it's not clear to me how we'd handle the case where we return Error anyway, so I think having the exception (at least, its on startup) is reasonable - so we can catch it right away and fix it. This seems like a 'fail fast' case to me.

Do you have an idea for alternative behavior?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bryphe my thinking was related to a user's workflow say I opened oni2 on a foreign system and for some strange reason we couldn't derive the home location, but I don't actually need the extension host I just want to view a file or do some other minor piece of work. I was thinking in such cases we might want to avoid an out right failure to boot I now have to debug, is it possible in this case that if we return an error we just shutdown or stop our attempts to use the host?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe the Environment.create could return Some or None. If None we just return unit somewhere rather than carry on any further or if we had an error record in state we could dispatch an error action so we could eventually render it somewhere so the user is aware that things have gone wrong and some stuff won't be working

Copy link
Member Author

@bryphe bryphe Mar 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my thinking was related to a user's workflow say I opened oni2 on a foreign system and for some strange reason we couldn't derive the home location,

Wouldn't this be a bug that we should fix? Shouldn't there always be a HOME location? We have lots of features that 'depend' on a HOME location - and we'll certainly have more as we support installing plugins, etc.

Two downsides I see with the proposal:

First - we'd be running oni2 in a degraded mode - sure, we might sidestep the first bug, but then we get bugs saying syntax highlighting doesn't work, or config doesn't load, or language extensions don't work, and that actually takes more time to debug than the 'fail fast' scenario. We end up with bugs that manifest further from the source.

Second - it propagates complexity down to other features - every feature that touches the home directory has to handle the case where it doesn't exist:

let openConfigurationFile = (effects: Effects.t, name) =>
  switch (Filesystem.createOniConfigFile(name)) {
  | Ok(path) => effects.openFile(~path, ())
  | Error(e) => print_endline(e)
  };

Wouldn't it be simpler just to establish the invariant that we have a home directory and downstream features can rely on that? If we can't establish that invariant - we fail fast, fix the bug, and maintain that invariant. We sidestep this extra complexity of features that use the home directory having to figure out alternate behavior if it doesn't exist. As a maintainer - that's appealing to me!

Copy link
Member

@akinsho akinsho Mar 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair point I can't see how this would fail in a way we could tolerate without a lot of other things breaking along the way, this case might be an exception (no pun intended).

It might be that the best thing is to completely fail but at the moment a runtime exception means the app closes abruptly with no indication.

I guess it's a larger issue than this specific function, I think failing fast is a good strategy.

But it would be good to consider these points as future places where we ought to pop up a dialog with some information then close the app etc.

switch (result)
  | Error(e) => {
     Revery.modal.show("Something bad happened" ++ e);
     failwith("Intolerable Exception")
  | Ok(value) => carryOn(value)
}

maybe intercept the exception warn the user then crash 🤷‍♂️ (just my two cents, not wanting/trying to be contrarian)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair point I can't see how this would fail in a way we could tolerate without a lot of other things breaking along the way, this case might be an exception (no pun intended).

😆

It might be that the best thing is to completely fail but at the moment a runtime exception means the app closes abruptly with no indication.

One thing I didn't call it out earlier is its important to differentiate between a crash like this on startup vs at runtime - runtime is so much worse, because it could mean the user lost data.

But it would be good to consider these points as future places where we ought to pop up a dialog with some information then close the app etc.

Yes - I think what we could do for now, is add a set of 'pre-flight' checks when we start the app. For now, it might be that the app just quits and shows an error. But once we have a 'Welcome screen' for #81 - that would be an even better place to show an error message, and we could have a nicer experience.

Thanks for the feedback and thoughts, @Akin909 !

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logged #193 to track the 'pre-flight' checks


let getOniDirectory: string => t(string);

let createOniConfigFile: string => t(string);
135 changes: 45 additions & 90 deletions src/editor/Extensions/ExtensionHostClient.re
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ open Oni_Core;
open Reason_jsonrpc;
/* open Revery; */

module Protocol = ExtensionHostProtocol;

type t = {
process: NodeProcess.t,
rpc: Rpc.t,
Expand All @@ -19,118 +21,71 @@ let emptyJsonValue = `Assoc([]);
type simpleCallback = unit => unit;
let defaultCallback: simpleCallback = () => ();

module Protocol = {
module MessageType = {
let initialized = 0;
let ready = 1;
let initData = 2;
let terminate = 3;
};

module LogLevel = {
let trace = 0;
let debug = 1;
let info = 2;
let warning = 3;
let error = 4;
let critical = 5;
let off = 6;
};

module Environment = {
type t = {
/* isExtensionDevelopmentDebug: bool, */
appRootPath: string,
appSettingsHomePath: string,
/* extensionDevelopmentLocationPath: string, */
/* extensionTestsLocationPath: string, */
globalStorageHomePath: string,
};

let create =
(~appRootPath, ~appSettingsHomePath, ~globalStorageHomePath, ()) => {
appRootPath,
appSettingsHomePath,
globalStorageHomePath,
};
};

/* module InitData { */
/* [@deriving (show, yojson({strict: false, exn: true}))] */
/* type t = { */
/* parentPid: int, */
/* environment: Environment.t, */
/* logLevel: int, */
/* logsLocationPath: string, */
/* autoStart: bool, */
/* }; */
/* }; */

module Notification = {
exception NotificationParseException(string);

type t = {
msgType: int,
reqId: int,
payload: Yojson.Safe.json,
};

let of_yojson = (json: Yojson.Safe.json) => {
switch (json) {
| `Assoc([
("type", `Int(t)),
("reqId", `Int(reqId)),
("payload", payload),
]) => {
msgType: t,
reqId,
payload,
}
| _ =>
raise(
NotificationParseException(
"Unable to parse: " ++ Yojson.Safe.to_string(json),
),
)
};
};
};
};

type messageHandler =
(int, Yojson.Safe.json) => result(option(Yojson.Safe.json), string);
let defaultMessageHandler = (_, _) => Ok(None);

let start =
(
~initData=ExtensionHostInitData.create(),
~onInitialized=defaultCallback,
~onMessage=defaultMessageHandler,
~onClosed=defaultCallback,
setup: Setup.t,
) => {
let args = ["--type=extensionHost"];
let env = [
"AMD_ENTRYPOINT=vs/workbench/services/extensions/node/extensionHostProcess",
"VSCODE_PARENT_PID=" ++ string_of_int(Unix.getpid()),
];
let process =
NodeProcess.start(~args, ~env, setup, setup.extensionHostPath);

let handleMessage = (id: int, _reqId: int, payload: Yojson.Safe.json) => {
switch (onMessage(id, payload)) {
| Ok(None) => ()
| Ok(Some(_)) =>
/* TODO: Send response */
()
| Error(_) =>
/* TODO: Send error */
()
let lastReqId = ref(0);
let rpcRef = ref(None);

let send = (msgType: int, msg: Yojson.Safe.json) => {
switch (rpcRef^) {
| None => prerr_endline("RPC not initialized.")
| Some(v) =>
incr(lastReqId);
let reqId = lastReqId^;

let request =
`Assoc([
("type", `Int(msgType)),
("reqId", `Int(reqId)),
("payload", msg),
]);

Rpc.sendNotification(v, "ext/msg", request);
};
};

let handleMessage = (id: int, _reqId: int, payload: Yojson.Safe.json) =>
if (id == Protocol.MessageType.ready) {
send(
Protocol.MessageType.initData,
ExtensionHostInitData.to_yojson(initData),
);
} else if (id == Protocol.MessageType.initialized) {
onInitialized();
} else {
switch (onMessage(id, payload)) {
| Ok(None) => ()
| Ok(Some(_)) =>
/* TODO: Send response */
()
| Error(_) =>
/* TODO: Send error */
()
};
};

let onNotification = (n: Notification.t, _) => {
switch (n.method, n.params) {
| ("host/msg", json) =>
open Protocol.Notification;
print_endline("[Extension Host Client] Unknown message: " ++ n.method);
print_endline("JSON: " ++ Yojson.Safe.to_string(json));
let parsedMessage = Protocol.Notification.of_yojson(json);
handleMessage(
Expand All @@ -145,8 +100,6 @@ let start =

let onRequest = (_, _) => Ok(emptyJsonValue);

/* let send = */

let rpc =
Rpc.start(
~onNotification,
Expand All @@ -156,6 +109,8 @@ let start =
process.stdin,
);

rpcRef := Some(rpc);

{process, rpc};
};

Expand Down
53 changes: 53 additions & 0 deletions src/editor/Extensions/ExtensionHostInitData.re
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* ExtensionHostInitData.re
*
* Module documenting the initialization data required for spinning up
* and activating the extension host
*/

open Oni_Core;

module ExtensionInfo = {
[@deriving (show({with_path: false}), yojson({strict: false, exn: true}))]
type t = {identifier: string};
};

module Workspace = {
[@deriving (show({with_path: false}), yojson({strict: false, exn: true}))]
type t = {__test: string};
};

module Environment = {
[@deriving (show({with_path: false}), yojson({strict: false, exn: true}))]
type t = {globalStorageHomePath: string};

let create = (~globalStorageHomePath=Filesystem.unsafeFindHome(), ()) => {
let ret: t = {globalStorageHomePath: globalStorageHomePath};
ret;
};
};

[@deriving (show({with_path: false}), yojson({strict: false, exn: true}))]
type t = {
extensions: list(ExtensionInfo.t),
parentPid: int,
environment: Environment.t,
logsLocationPath: string,
autoStart: bool,
};

let create =
(
~parentPid=Unix.getpid(),
~extensions=[],
~environment=Environment.create(),
~logsLocationPath=Filesystem.unsafeFindHome(),
~autoStart=true,
(),
) => {
parentPid,
extensions,
environment,
logsLocationPath,
autoStart,
};
69 changes: 69 additions & 0 deletions src/editor/Extensions/ExtensionHostProtocol.re
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* ExtensionHostProtocol.re
*
* This module documents & types the protocol for communicating with the VSCode Extension Host.
*
*/

module MessageType = {
let initialized = 0;
let ready = 1;
let initData = 2;
let terminate = 3;
let requestJsonArgs = 4;
};

module LogLevel = {
let trace = 0;
let debug = 1;
let info = 2;
let warning = 3;
let error = 4;
let critical = 5;
let off = 6;
};

module Environment = {
type t = {
/* isExtensionDevelopmentDebug: bool, */
/* appRootPath: string, */
/* appSettingsHomePath: string, */
/* extensionDevelopmentLocationPath: string, */
/* extensionTestsLocationPath: string, */
globalStorageHomePath: string,
};

let create = (~globalStorageHomePath, ()) => {
globalStorageHomePath;
};
};

module Notification = {
exception NotificationParseException(string);

type t = {
msgType: int,
reqId: int,
payload: Yojson.Safe.json,
};

let of_yojson = (json: Yojson.Safe.json) => {
switch (json) {
| `Assoc([
("type", `Int(t)),
("reqId", `Int(reqId)),
("payload", payload),
]) => {
msgType: t,
reqId,
payload,
}
| _ =>
raise(
NotificationParseException(
"Unable to parse: " ++ Yojson.Safe.to_string(json),
),
)
};
};
};
1 change: 1 addition & 0 deletions src/editor/Extensions/Oni_Extensions.re
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ module ColorMap = ColorMap;
module ColorizedToken = ColorizedToken;
module ExtensionContributions = ExtensionContributions;
module ExtensionHostClient = ExtensionHostClient;
module ExtensionHostInitData = ExtensionHostInitData;
module ExtensionManifest = ExtensionManifest;
module ExtensionScanner = ExtensionScanner;
module TextmateClient = TextmateClient;
5 changes: 4 additions & 1 deletion src/editor/bin/Oni2.re
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,10 @@ let init = app => {
grammars,
);

let extHostClient = Extensions.ExtensionHostClient.start(setup);
let onExtHostClosed = () => print_endline("ext host closed");

let extHostClient =
Extensions.ExtensionHostClient.start(~onClosed=onExtHostClosed, setup);

Extensions.TextmateClient.setTheme(tmClient, defaultThemePath);

Expand Down
20 changes: 6 additions & 14 deletions test/editor/Extensions/ExtensionClientTest.re
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,20 @@ open Oni_Extensions;
open TestFramework;

describe("Extension Client", ({test, _}) =>
test("receive init message", ({expect}) =>
test("gets initialized message", ({expect}) =>
Helpers.repeat(() => {
let setup = Setup.init();

let gotReadyMessage = ref(false);
let initialized = ref(false);

let onClosed = () => ();
let onMessage = (id, _) => {
if (id === ExtensionHostClient.Protocol.MessageType.ready) {
gotReadyMessage := true;
};

Ok(None);
};

let extClient = ExtensionHostClient.start(~onClosed, ~onMessage, setup);
let onInitialized = () => initialized := true;
let extClient = ExtensionHostClient.start(~onInitialized, setup);

Oni_Core.Utility.waitForCondition(() => {
ExtensionHostClient.pump(extClient);
gotReadyMessage^;
initialized^;
});
expect.bool(gotReadyMessage^).toBe(true);
expect.bool(initialized^).toBe(true);
})
)
);
2 changes: 1 addition & 1 deletion test/editor/dune
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
(libraries
yojson
Oni_Core_Test
Oni_Model_Test
Oni_Extensions_Test
Oni_Model_Test
Oni_Neovim_Test
))

Expand Down