fix: offline error message when node is shutting down #5797
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Performance Report✔️ no performance regression detected Full benchmark results
|
nazarhussain
changed the title
nflaig
reviewed
Jul 31, 2023
There was a problem hiding this comment.
As the EL status changes rely on error.code returned from fetch, we might wanna merge #5811 first to make sure EL status still works and adapt error codes here in this PR if required.
I added a bunch of tests in the PR to make sure error codes still work as expected, see fetch.test.ts.
Based on codes defined here
lodestar/packages/beacon-node/src/execution/engine/utils.ts
Lines 39 to 40 in 59ec5fe
It seems like EAI_AGAIN, ECONNRESET, and ECONNABORTED are not covered, @nazarhussain if by any chance you know how to produce them I would add them to the test cases.
| import {ErrorJsonRpcResponse, HttpRpcError} from "../../../../src/eth1/provider/jsonRpcHttpClient.js"; | ||
|
|
||
| describe("execution/engine/utils", () => { | ||
| describe("getExecutionEngineState", () => { |
There was a problem hiding this comment.
These tests are great!
As this relies on error.code returned from fetch, we might wanna merge #5811 first to make sure EL status is still works.
Tests would not catch if error.code returned from fetch changes, will have to cover all cases in fetch.test.ts to make sure we can rely on those error codes.
There was a problem hiding this comment.
These are very standard error codes must comply with any fetch implementation. Testing regressively native implementations is not suggested. But for node/browser compatibility of fetch we should have some basic tests.
There was a problem hiding this comment.
Yes, I doubt the error codes will ever change, I just tested different cases in regard to custom FetchError added in #5811 but these are rather meant to test if FetchError works as expected.
Since we use native fetch now it is even more unlikely we get unexpected errors. As per my understanding the codes you are checking right now for EL offline/auth failed should be alright and it is anyhow hard produce specific error codes (#5811 (comment)).
For those e need to create a custom server implementation which reset the client connections. I don't think it's important enough to mock a server for now. |
g11tech
reviewed
Aug 5, 2023
g11tech
reviewed
Aug 5, 2023
g11tech
reviewed
Aug 5, 2023
g11tech
reviewed
Aug 5, 2023
| } | ||
| } | ||
|
|
||
| function getExecutionEngineStateForPayloadError( |
There was a problem hiding this comment.
Suggested change
| function getExecutionEngineStateForPayloadError( | |
| function getELStateFromPayloadError( |
g11tech
reviewed
Aug 5, 2023
| ) | ||
| // If there are errors by EL like connection refused, internal error, they need to be | ||
| // treated separate from being INVALID. For now, just pass the error upstream. | ||
| .catch((e: Error): EngineApiRpcReturnTypes[typeof method] => { |
There was a problem hiding this comment.
this error handling is required upstream in processBlocks (as well as to not downgrade the peer in case of UNAVAILABLE)
g11tech
requested changes
Aug 5, 2023
nazarhussain
force-pushed
the
nh/fix-el-status-message
branch
from
65d84c2
to
9ade9c4
Compare
g11tech
reviewed
Aug 7, 2023
| @@ -30,7 +30,7 @@ export enum ExecutePayloadStatus { | |||
| UNSAFE_OPTIMISTIC_STATUS = "UNSAFE_OPTIMISTIC_STATUS", | |||
| } | |||
|
|
|||
| export enum ExecutionEngineState { | |||
| export enum ExecutionState { | |||
There was a problem hiding this comment.
i am thinking if ExecutionState might cause confusion over reference to actual execution state, in that case original ExecutionEngineState is preferable
There was a problem hiding this comment.
If I use ExecutionEngineState, then would prefer to use the same vocabulary/wording to other places for variables and functions. That will lead to almost similar to what we had earlier. Will it be ok?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
There should not be a false error log.
Description
Currently when node is shutting down, it shows an error message that EL engine is turned offline, which is not the valid case.
Steps to test or reproduce