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

Refactor feature/proc #94

Merged
merged 9 commits into from
Mar 20, 2023
Merged

Refactor feature/proc #94

merged 9 commits into from
Mar 20, 2023

Conversation

lthibault
Copy link
Collaborator

@lthibault lthibault commented Mar 14, 2023

@mikelsr As warned, this ended up being a pretty large diff.

Changes include:

  • package restructuring
  • renaming of various symbols (not 100% convinced about the process.Server name; let me know what you think).
  • eschew process constructor for config/factory pattern in pkg/server
  • remove IO streams, as per today's discussion
  • change host code to return a non-zero exit code
  • add unit test

Let me know what you think!

P.S. It might be easier to checkout the cleanup/proc-lazy-init branch and just read that directly. The diff is super noisy (though much of it is file renames/deletes). If you decide to do this, running the unit test is probably a good place to start.

@lthibault lthibault added the enhancement New feature or request label Mar 14, 2023
@lthibault lthibault added this to the 0.1.0 Public Beta Release milestone Mar 14, 2023
@lthibault lthibault requested a review from mikelsr March 14, 2023 21:25
@lthibault lthibault changed the title Cleanup/proc lazy init Refactor feature/proc Mar 14, 2023
Copy link
Contributor

@mikelsr mikelsr left a comment

Choose a reason for hiding this comment

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

I've learnt quite a lot seeing what you did in this refactor! It was refreshing

msg @1 :Text;
exitErr :group {
code @2 :UInt32;
module @3 :Text;
Copy link
Contributor

Choose a reason for hiding this comment

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

What does module represent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, this is the name of the module. It is one of the fields contained in sys.ExitError, which is returned by calls to exported module functions. (Maybe we should rename to moduleName?)

pkg/process/executor.go Show resolved Hide resolved
pkg/process/executor_test.go Show resolved Hide resolved
return err
}

ee := state.Err.(*sys.ExitError)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just so we are in the same page, will error be used for when something goes wrong outside the process execution and ExitError for errors raised during execution? If so I like it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct -- returning an error from a capability server's method means "the RPC call failed". The capnp Error struct means "an application-level error occurred". In the present case, that indeed means the guest code returned an error.

This is exactly equivalent to HTTP, where error means the network-level request/response failed, and a status code can indicate an error in the application logic. It's just more expressive than returning integer codes.

pkg/process/process.go Outdated Show resolved Hide resolved
@lthibault lthibault requested a review from mikelsr March 16, 2023 18:58
@mikelsr mikelsr merged commit 5fd594e into feature/process Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants