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

Error messages refactoring #1854

Merged
merged 61 commits into from
Jan 22, 2020
Merged

Error messages refactoring #1854

merged 61 commits into from
Jan 22, 2020

Conversation

lexfrl
Copy link
Contributor

@lexfrl lexfrl commented Dec 12, 2019

#1839

  • All runtime errors implements serde::Serialize
  • Struct errors shouldn't use tuple structs
  • add an index of the failed Action for ActionError
  • FunctionCall error must be structured
  • FunctionCall in view?

closes #1288, #1839

Copy link
Contributor

@MaksymZavershynskyi MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

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

Maybe we should also rename near-vm-errors to near-runtime-errors.

@@ -211,21 +233,18 @@ impl JsonRpcHandler {
let result = self
.client_addr
.send(NetworkClientMessages::Transaction(tx))
.map_err(|err| RpcError::server_error(Some(convert_mailbox_error(err))))
Copy link
Contributor

Choose a reason for hiding this comment

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

@bowenwang1996 @frol In what cases RPC will return a MailboxError? AFAIU it only happens if the communication between the actors has timed out or the channel has been closed. The latter one can happen when the node is shutting down (btw, we need to implement a way to gracefully shutdown the node). The former one is not supposed to happen in our infra, since the communication between the actors does not have a timeout , correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I think this error can only happen under extreme circumstances, but @evgenykuzyakov insisted that I handle it. I think it can also happen if the node receives too much traffic.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. @bowenwang1996 is it correct that communication between our Actix actors uses bounded channels that can overflow and drop the messages? Is it possible to arrive to an unforeseen state due to some messages being lost between the Actix actors? CC @SkidanovAlex

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure. I think @frol knows actix better, but we in general should be resistant to lost messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like Actix uses unbounded channels by the default, so it should be good unless we explicitly changed it. actix/actix#14

Copy link
Collaborator

Choose a reason for hiding this comment

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

unbounded channels are the major source of unbounded memory consumption, and they tend to hide problems until it is too late.

core/primitives/src/errors.rs Outdated Show resolved Hide resolved
@lexfrl
Copy link
Contributor Author

lexfrl commented Dec 20, 2019

Maybe we should also rename near-vm-errors to near-runtime-errors.

I agree.

@lexfrl
Copy link
Contributor Author

lexfrl commented Dec 23, 2019

Maybe we should also rename near-vm-errors to near-runtime-errors.

On a second though, near-vm-errors are essentially FunctionCall errors.. I'm not sure we should to rename it now. All "runtime errors" are in near-primitives ATM. And all the VM errors are in near-runtime.

core/primitives/src/errors.rs Outdated Show resolved Hide resolved
core/primitives/src/errors.rs Outdated Show resolved Hide resolved
core/primitives/src/errors.rs Outdated Show resolved Hide resolved
core/primitives/src/errors.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@MaksymZavershynskyi MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

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

Tracking issue #1972

tools/rpctypegen/Cargo.toml Outdated Show resolved Hide resolved
scripts/build_errors_schema.sh Outdated Show resolved Hide resolved
pub props: BTreeMap<String, String>,
}

thread_local!(static SCHEMA: RefCell<Schema> = RefCell::new(Schema::default()));
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make sure procedural macroses are never executed by different threads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rustc is a single-threaded AFAIK. I guess we are about to came up with the more robust solution in #1972 anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

It definitely runs parallelizes compilation of the crates across the processes or threads (I don't remember exactly, but you can see it for yourself in htop when you compile nearcore) so this schema is not going to be reused across the crates.

Copy link
Contributor Author

@lexfrl lexfrl Jan 20, 2020

Choose a reason for hiding this comment

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

I meant rustc for a single crate is single-threded, but not an entire build process, sure. That's why it merges into the json file if the file exists. Of course it's far from ideal: there is a small chance that it will rewrite the file. Since it's already a hack (and will be rewritten at some point) and since it's not a runtime code I don't think it worth to make it perfect. If you have a better idea at hand - I'd be glad to apply it!

runtime/near-vm-errors/src/lib.rs Show resolved Hide resolved
tools/rpctypegen/src/lib.rs Outdated Show resolved Hide resolved
Co-Authored-By: Maksym Zavershynskyi <[email protected]>
Copy link
Collaborator

@evgenykuzyakov evgenykuzyakov left a comment

Choose a reason for hiding this comment

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

Approving. Time to ship :)

near-rpc-error-macro = { path = "../../tools/rpctypegen/macro" }

[build-dependencies]
_rand = { package = "rand", version = "0.6.5" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need this specific version of rand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a trick to make it compile. Take a look:

dalek-cryptography/ed25519-dalek#89

@MaksymZavershynskyi
Copy link
Contributor

Events are now merged. This PR is unblocked.

Co-Authored-By: Evgeny Kuzyakov <[email protected]>
@lexfrl
Copy link
Contributor Author

lexfrl commented Jan 22, 2020

@nearmax your approval is needed

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.

6 participants