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

Remote: abstract receiving from remote process; to replace for all remote process error/data handlers #365

Merged
merged 1 commit into from Oct 8, 2015

Conversation

rwaldron
Copy link
Contributor

@rwaldron rwaldron commented Oct 5, 2015

As t2-cli has evolved, we've built up some technical debt w/r to handling remote process io. Specifically, the code that handled receiving errors, data and closing was repeated in several places. This PR abstracts the operation into a Promise returning method of the current Tessel instance.

});
});
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnnyman727 start here.

@rwaldron
Copy link
Contributor Author

rwaldron commented Oct 6, 2015

I'll get those failures fixed in the morning

@johnnyman727
Copy link
Contributor

@rwaldron I don't quite understand the purpose of this PR. It seems like it abstracts out part of tessel.simpleExec so that it's less encapsulated for instances where simpleExec was too abstract?

@rwaldron rwaldron changed the title Remote: abstract "forwarding", to replace for all remote process error/data handlers Remote: abstract receiving from remote process; to replace for all remote process error/data handlers Oct 6, 2015
@rwaldron
Copy link
Contributor Author

rwaldron commented Oct 6, 2015

I don't quite understand the purpose of this PR. It seems like it abstracts out part of tessel.simpleExec so that it's less encapsulated for instances where simpleExec was too abstract

No, simpleExec resolves with remoteProcess, what is then done with that remoteProcess is arbitrary, but always involved:

  1. listening to remoteProcess.stderr for "data" events
    • capturing any error data as it's received
  2. listening to remoteProcess.stdout for "data" events
    • capturing any data as it's received
  3. sometimes writing to remoteProcess.stdin
  4. listening to remoteProcess for "close" events
    • If any error data was captured, reject(...)
    • Otherwise, resolve(...), sometimes with the data captured in step 2.
    • Sometimes log some message

There were 5 places were either all 4 steps were taken, or some combination eg. 1 & 4 or 1, 3, & 4 or just 4. All 5 places had slightly different variable names and slightly different comments explaining the operation—but all were doing either exactly (or very nearly) the same thing. Abstracting the intersection semantics into a single operation (steps 1, 2, 4), that returns a promise which is either resolved with any received data or rejected with any error, provides a sound consistency across all occurrences. This cannot be rolled into simpleExec, because of step 3, where sometimes remoteProcess.stdin needs to be written to.

All of the affected operations were smoke tested many times, with different T2 devices.

@johnnyman727
Copy link
Contributor

@rwaldron so, do I understand it correctly that receive is essentially "let me create the process and do something to it but just let me know when it has completed" whereas "simpleExec" is equivalent to "just run the command and tell me when it completes - I'm not going to write to the process"?

@johnnyman727
Copy link
Contributor

I think it was the name receive that through me off but now I get it. It's when you're done with a process and want to receive data and close.

In any case, the PR looks good to me so feel free to merge!

rwaldron added a commit that referenced this pull request Oct 8, 2015
Remote: abstract receiving from remote process; to replace for all remote process error/data handlers
@rwaldron rwaldron merged commit 8b66a7a into tessel:master Oct 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants