-
Notifications
You must be signed in to change notification settings - Fork 660
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
Conversation
d9e6953
to
775c141
Compare
5f2226f
to
775c141
Compare
There was a problem hiding this 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)))) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
I agree. |
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. |
8027ee5
to
08f601f
Compare
66331ed
to
c641f4a
Compare
There was a problem hiding this 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/src/lib.rs
Outdated
pub props: BTreeMap<String, String>, | ||
} | ||
|
||
thread_local!(static SCHEMA: RefCell<Schema> = RefCell::new(Schema::default())); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
Co-Authored-By: Maksym Zavershynskyi <[email protected]>
There was a problem hiding this 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" } |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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:
Events are now merged. This PR is unblocked. |
Co-Authored-By: Evgeny Kuzyakov <[email protected]>
@nearmax your approval is needed |
#1839
closes #1288, #1839