-
Notifications
You must be signed in to change notification settings - Fork 806
Fixed Ganache loading forever being stuck due to project details not loading #1382
Fixed Ganache loading forever being stuck due to project details not loading #1382
Conversation
Wow. Thanks for digging in and finding the issue! Very happy to learn I was wrong about the root cause. |
…der never resolves/rejects The project loader is performed using a child spawn that should resolved or rejects at some point. However, it seems that in some cases (when some dependencies of the truffle project are missing), the child spawn will issue an 'exit` event without ever entering the 'error` event. This is problematic because since this is not trapped properly, the actual Ganache dependency that is waiting for project loading to happen never resolves and the Ganache UI is loading forever being stuck in this loop. To resolves that, the 'exit' signal is now catched as well as the standard error output of the process. When the child process exits with a bad error code and as now received the `error` event, a message is now forwarded within an `Error` object and the standard error output is present in there. This removes the infinite loop in Ganache and shows the actual standard error in the process. Caveats, this renders as 'Oups this is a bug' dialog in Ganache while it's really a problem with the truffle project not being configured correctly. I think it's good enough as a first solving, but this could be improved later on be provided a different UI saying the project is misconfigured or something like this.
e3bdd02
to
5ac0f46
Compare
… into fix/fetch-project-details-never-resolving
if (truffleIntegration) { | ||
await truffleIntegration.stopWatching(); | ||
} | ||
|
||
if (chain.isServerStarted()) { | ||
// Something wrong happened in the chain, let's try to stop it | ||
if (mainWindow) { |
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 was possible for the error
here to cause the UI to render before the UI had all the data it needs to render. This change makes sure the chain is ready before sending the error to the mainWindow
.
@@ -8,6 +8,7 @@ | |||
left: 0px; | |||
right: 0px; | |||
width: 100%; | |||
overflow: auto; |
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.
These were some drive-by fixes to CSS layout issues. Mainly due to the new error message that can cause the screen to overflow the y axis.
} | ||
|
||
.RowItem { | ||
width: 50%; | ||
width: calc(50% - 1rem); |
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.
The expanding stack trace section of the error message was causing a jump. 50% was a lie due to css's padding rules. Using calc
and justify-content: space-between;
keeps things from jumping around (and makes the two columns actually equal in width)
@@ -199,6 +195,13 @@ | |||
border-bottom: 1px solid; | |||
border-bottom-color: rgba(0, 0, 0, 0.2); | |||
|
|||
button { |
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.
Just special styling of button
s that appear in .ValidationError
s. There is only one use case so far.
<div className="WorkspaceProjects"> | ||
<div className="projectItemContainer"> | ||
{projects.map((path, idx) => { | ||
{(projects || []).map((path, idx) => { |
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've found that projects
can be undefined
sometimes, likely due to a race condition. I didn't want to dive in and fix this one, as I think it needs a pretty large re architecture to fix it right.
</div> | ||
<br /> | ||
</div> | ||
)} |
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 feel like this is not idiomatic React. Comments appreciated!
@@ -1,3 +1,4 @@ | |||
const { encodeEventSignature } = require("web3-eth-abi"); |
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.
All the changes in this file are already in develop (see trufflesuite/ganache@4f8c534#diff-4e386a10b26b7224f48e8ee5a9652a3cR1-R129) and I can't seem to make them easily go away from this PR. You can ignore all changes in this file.
package.json
Outdated
@@ -248,7 +249,8 @@ | |||
"truffle-decoder": "^3.0.8", | |||
"universal-analytics": "^0.4.13", | |||
"uuid": "^3.1.0", | |||
"web3": "1.2.1" | |||
"web3": "1.2.1", | |||
"web3-eth-abi": "1.2.1" |
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.
This change is already in develop. /shrug
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.
@davidmurdoch added a few comments.
@@ -120,9 +122,43 @@ class WorkspaceScreen extends Component { | |||
<h4>TRUFFLE PROJECTS</h4> | |||
<div className="Row"> | |||
<div className="RowItem"> | |||
{this.props.config.validationErrors["workspace.project"] && ( |
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.
This is pretty hard to read, might be worthwhile to assign this to a more semantic variable first, like:
const validationErrors = this.props.config.validationErrors["workspace.project"]
And then use it like:
{validationErrors && <div>...</div>}
This would also simplify line 129 to simply be: validationErrors.message
<OnlyIf test={!this.state.showErrorDetails}> | ||
show stack trace | ||
</OnlyIf> | ||
</button> |
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.
6 lines for a conditional button text with usage of a custom <OnlyIf>
component is a bit overkill. You could easily do something like:
<button>
{this.state.showErrorDetails ? "hide stack trace" : "show stack trace"}
</button>
} | ||
<button | ||
onClick={() => { | ||
this.setState({ |
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 wonder if this could be a class method instead, to preserve readability:
toggleErrorDetails = () => this.setState({ showErrorDetails: !this.state.showErrorDetails })
And then your button can just be:
<button onClick={this.toggleErrorDetails}>
].stack.map(line => line.toString())} | ||
</SyntaxHighlighter> | ||
</div> | ||
</OnlyIf> |
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.
Again, avoid use of the <OnlyIf>
component. You can use short-circuiting here:
{this.state.showErrorDetails && <div>...</div>}
<SyntaxHighlighter language="bash"> | ||
{this.props.config.validationErrors[ | ||
"workspace.project" | ||
].stack.map(line => line.toString())} |
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.
If we followed the above suggestion to name validationErrors
, we could change this to:
validationErrors.stack.map(line => line.toString())
…ading (#1382) Fixed Ganache loading forever being stuck because project details loader never resolves/rejects The project loader is performed using a child spawn that should resolved or rejects at some point. However, it seems that in some cases (when some dependencies of the truffle project are missing), the child spawn will issue an 'exit` event without ever entering the 'error` event. This is problematic because since this is not trapped properly, the actual Ganache dependency that is waiting for project loading to happen never resolves and the Ganache UI is loading forever being stuck in this loop. To resolves that, the 'exit' signal is now catched as well as the standard error output of the process. When the child process exits with a bad error code and as now received the `error` event, a message is now forwarded within an `Error` object and the standard error output is present in there. This removes the infinite loop in Ganache and shows the actual standard error in the process.
@davidmurdoch I talked to you yesterday (Friday August 2nd) at Trufflecon about this loading problem. I encountered it again in while attending the RSK workshop in the afternoon
The project loader is performed using a child spawn that should resolved or rejects at some point. However, it seems that in some cases (when some dependencies of the truffle project are missing), the child spawn will issue an 'exit
event without ever entering the 'error
event.This is problematic because since this is not trapped properly, the actual Ganache dependency that is waiting for project loading to happen never resolves and the Ganache UI is loading forever being stuck in this loop.
To resolves that, the 'exit' signal is now caught as well as the standard error output of the process. When the child process exits with a bad error code and as now received the
error
event, a message is now forwarded within anError
object and the standard error output is present in there.This removes the infinite loop in Ganache and shows the actual standard error in the process.
Caveats, this renders as
Oups this is a bug
dialog in Ganache while it's really a problem with the truffle project not being configured correctly. I think it's good enough as a first solving, but this could be improved later on be provided a different UI saying the project is misconfigured or something like this.Here the error that happened to me in the child process while trying to load the truffle project:
It was an easy fix once the message was shown up.