Skip to content
This repository was archived by the owner on Jul 23, 2019. It is now read-only.

Switch to a client/server architecture #46

Merged
merged 56 commits into from
Mar 22, 2018
Merged

Switch to a client/server architecture #46

merged 56 commits into from
Mar 22, 2018

Conversation

nathansobo
Copy link

@nathansobo nathansobo commented Mar 16, 2018

As discussed in the update for March 12, we are switching Xray to a client/server architecture that places as much logic as possible in a central server process. This PR is the implementation of that plan.

I'm including a detailed explanation of the contents of this PR in a markdown document that will become a permanent part of our documentation once this is merged. Hopefully it's high-level enough that we have a reasonable chance of keeping it updated.

Tasks

  • Get basic window-launching flow working
  • Send view updates to the client
  • Send actions to the server
  • Clean up documentation
  • Address double borrow when dropping ViewHandles
  • Eliminate remaining compiler warnings

@cmyr, since you were interested in any PRs related to our approach to the client/server protocol, I'm cc'ing you on this PR.

This was referenced Mar 19, 2018
@nathansobo nathansobo force-pushed the server branch 3 times, most recently from 46f8635 to 698496a Compare March 19, 2018 21:26
@lucat1
Copy link

lucat1 commented Mar 20, 2018

I would like to test the new architecture, but I get stuck at building the napi module via the scripts/napi.js script. Could you provide any build instructions for the new setup? Maybe in the README.

BTW here' the error I get when running the script:

Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
cp: ../target/debug/libnapi.dylib: No such file or directory
child_process.js:614
    throw err;
    ^

Error: Command failed: cp ../target/debug/libnapi.dylib target/debug/napi.node
at checkExecSyncError (child_process.js:574:11)
at Object.execSync (child_process.js:611:13)
at Object. (/Users/luca/projects/xray/napi/scripts/napi.js:55:8)
at Module._compile (module.js:660:30)
at Object.Module._extensions..js (module.js:671:10)
at Module.load (module.js:573:32)
at tryModuleLoad (module.js:513:12)
at Function.Module._load (module.js:505:3)
at Function.Module.runMain (module.js:701:10)
at startup (bootstrap_node.js:190:16)


After a quick debug I've noticed that the script isn't generating the .dylib file, that it should: https://github.com/atom/xray/blob/master/napi/scripts/napi.js

@alexandernst
Copy link

A wild idea: Would it be possible to completely split client from server (two different binaries) and make them do IPC via socket (only when required, for performance reasons).

If that is possible, then I think it would be possible to install Atom-server on a remote machine (let's say a production server without X) and Atom-client on a local machine (let's say developer's machine, like for example random John Doe that shouldn't be editing files remotely on the production server).

@nathansobo
Copy link
Author

@alexandernst We are completely separating the client from the server in this PR as you describe, but the server needs to be running locally for there to be acceptable latencies. We do intend to enable server to server communication to enable the kind of experience you're describing, but we can't just connect the view directly to a remote server without unacceptable lag in the UX.

@NachE
Copy link

NachE commented Mar 20, 2018

This sounds like a 21st century Xfree. Do you expect good latency using client/server architecture?

@nathansobo
Copy link
Author

@NachE if the server is local as we’re currently designing this then yes, latency should be minimal. Communications with a remote server will be routed through the local server and use CRDTs to avoid latency where it would be problematic.


![Window protocol diagram](../images/window_protocol.png)

The protocol between the window and the server in inspired by the [Flux application architecture](https://facebook.github.io/flux/), though it's probably different in some ways due to the particular needs of Xray.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/in inspired by/is inspired by

Copy link
Author

Choose a reason for hiding this comment

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

🙇

Nathan Sobo and others added 12 commits March 22, 2018 03:54
Co-authored-by: Max Brunsfeld <[email protected]>
Co-authored-by: Nathan Sobo <[email protected]>
In Client::poll, we need to loop when polling the socket and the channel
until we receive a NotReady event. This ensures that the parent task is
woken up again when we receive more input.

Co-authored-by: Antonio Scandurra <[email protected]>
This is a more OO approach that simplifies the interface of
message-handling methods.

Co-authored-by: Antonio Scandurra <[email protected]>
Co-authored-by: Antonio Scandurra <[email protected]>
Nathan Sobo and others added 15 commits March 22, 2018 03:54
This ensures that the future that forwards window updates is dropped
when the window client disconnects.

Co-authored-by: Antonio Scandurra <[email protected]>
If a new window client appears, we create a new window update stream and
cause any previous update streams to terminate.
Co-authored-by: Max Brunsfeld <[email protected]>
We won't have much that isn't a component, so this makes things less
verbose. Worst case we can put all the components in a folder.

Co-authored-by: Max Brunsfeld <[email protected]>
We did a bunch of other changes in this commit to the styling framework
while debugging an issue. We're just going to roll with it.

Co-authored-by: Max Brunsfeld <[email protected]>
This way, the view can call `add_view` inside of `did_mount` without
double-borrowing the window Inner.
Also, remove the `workspace` field on Windows and simply add the
workspace as the window's root view from the outside.
The call to setState was causing the editor to render twice for each
scroll event: once due to the state update and a second time shortly
afterward due to the message from the server.
Co-authored-by: Nathan Sobo <[email protected]>
Co-authored-by: Antonio Scandurra <[email protected]>
Nathan Sobo and others added 4 commits March 22, 2018 04:43
Co-authored-by: Antonio Scandurra <[email protected]>
Co-authored-by: Antonio Scandurra <[email protected]>
Now that we use a client/server architecture, we don't need bindings
into V8. We may resurrect this code later if we want to provide bindings
to an embedded V8 VM in the server.

Co-authored-by: Antonio Scandurra <[email protected]>
@nathansobo
Copy link
Author

We've changed the roadmap on this PR a bit to cut scope. Previously, we were thinking of including the file finder experience as part of this PR. Thinking about the file finder was useful in driving out the design of our window / views abstraction, and we made decent progress toward implementing it, but we've decided to drop it from this PR for now and just render an example editor. That gets us back to parity with master. We should be able to merge this week, and then we can pick up the file finder work on another branch.

@nathansobo nathansobo merged commit 0035c7f into master Mar 22, 2018
@nathansobo nathansobo deleted the server branch March 22, 2018 16:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants