fix: blockchain logs show in cockpit #1367
Conversation
There was a problem hiding this comment.
Hate to do this to you man, but when you get a chance tomorrow, could you please add a bit of a description for what the problem was and how it was fixed? Just because it was pretty confusing to work out what the issue was, it may help us down the line when we forget again 😃
packages/embark/src/lib/core/ipc.js
Outdated
| @@ -118,6 +118,10 @@ class IPC { | |||
| cb = cb || (() => {}); | |||
| return cb(); | |||
| } | |||
| this.queueRequest(action, data, cb); | |||
There was a problem hiding this comment.
We don't really need this anymore do we? 😄
There was a problem hiding this comment.
Good point. Fixing.
andremedeiros
force-pushed
the
fix/blockchain-logs-in-cockpit
branch
from
866865f to
75665c1
Compare
I was pretty burnt out after waking up at 3 am last night. Sorry for not adding a description in the first place. Fixed! |
| @@ -135,10 +139,13 @@ Blockchain.prototype.initStandaloneProcess = function () { | |||
| // `embark run` without restarting `embark blockchain`) | |||
| setInterval(() => { | |||
There was a problem hiding this comment.
Rather than use setInterval, my preference would be for a recursive setTimeout. Reason: although unlikely, because the interval is 2000ms, there is a possibility of overlapped calls of this.ipc.connect, i.e. if for some reason the previous invocation's callback hasn't fired yet.
There was a problem hiding this comment.
That makes sense, agreed.
#1367 introduced a change that prevented the `blockchain_listener` from listening to logs until the IPC server connected. However, the restriction went too far, and included restricting registration of console commands and API endpoints for the regular transactions feature. This fix reverts the changes that affect registration of regular txs endpoints and console commands.
#1367 introduced a change that prevented the `blockchain_listener` from listening to logs until the IPC server connected. However, the restriction went too far, and included restricting registration of console commands and API endpoints for the regular transactions feature. This fix reverts the changes that affect registration of regular txs endpoints and console commands.
There are two ways to launch the blockchain process with Embark: standalone, by running
embark blockchain, or with the dashboard, by runningembark run.When Embark runs, it also starts an IPC server that is used, amongst other things, to receive logs from potentially the standalone process. The problem is that the log listener for the standalone blockchain process starts independently of the blockchain process launcher, which will try to start a blockchain process even if one is already running. Each of these also registers an API endpoint which is supposed to serve logs. As the API endpoint is the same for both, one overwrites the other, and in this case
BlockchainListeneralways won.This PR sets things up in a way that
BlockchainListenerwill only register its API endpoint if, and only if, an IPC connection occurs. The standalone blockchain process has a retry loop that will attempt to connect to IPC every so often, so once it connects, it will now send the logs that IPC missed plus any future logs.You suck, @emizzle