Start incrementing jsonrpc message id from random number #5371
Conversation
|
Your Render PR Server URL is https://web3-js-pr-5371.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-cc1urp9gp3jp25qfgh6g. |
Muhammad-Altabba
changed the title
Muhammad-Altabba
requested review from
nikoulai,
luu-alex,
jdevcs,
avkos,
nazarhussain and
spacesailor24
and removed request for
nikoulai
Pull Request Test Coverage Report for Build 2912165598
💛 - Coveralls |
|
Looks good 👍 is this something we want to consider for 4.x as well? |
| messageId: 0 | ||
| // This is the starting counter for the Jsonrpc.id. And, to insure staying in the safe range while incrementing later, | ||
| // pick a random number between 0 and (the maximum safe integer minus the max unsigned 32 bit integer) | ||
| messageId: Math.floor(Math.random() * (Number.MAX_SAFE_INTEGER - Math.pow( 2, 32 ))) |
There was a problem hiding this comment.
I had a hunch that this will not fix the actual issue.
Responses get mixed up due to conflicting payload IDs
The response will still be mixed up if the payload Ids are conflicting, starting point does not matter and randomizing starting point would minimize but not fixed the problem.
For once instance of the web3 there will be no conflicting payload IDs in any case.
There was a problem hiding this comment.
As per here: https://www.jsonrpc.org/specification
An identifier established by the Client that MUST contain a String, Number, or NULL value if included.
uuid is already used in 1.x, maybe we could use it, also, for request id`s. We won't need a starting point and other libraries use numbers for ids.
There was a problem hiding this comment.
Yes, there will be no issue if one instance is running and one library is used. And, yes, this solution will minimize the possibility of a conflict to some grade that is neglected but the possibility is still there.
But using a uuid (that would be handled as a string) instead of a number would be a breaking change. Even the tests for 1.x would need to be modified accordingly.
However, I proposed to handle this in a different way in v4.x as there we can do a breaking change by making the message id as a string that contains a UUID or another format. The new issue is here: #5373
And if you have a better suggestion to solve this in v1.x, please advise.
Thanks,
There was a problem hiding this comment.
We can proceed with this approach but considering it's not a permanent fix.
Also it does not matter if there is more than one library using the same id for the same endpoint. For the HTTP, the RPC server will be responding synchronously. For WS based RPC server it will be responding per connection. So if there are two libraries, both would be having different connections and that will not conflict the ids.
There was a problem hiding this comment.
Thanks @nazarhussain ,
As you see this as "not a permanent fix", do you have a better suggestion for 1.x that does not cause a breaking change?
There was a problem hiding this comment.
So if there are two libraries, both would be having different connections and that will not conflict the ids.
@nazarhussain I just wanted to note in my original report #5327, I saw a situation where two libraries were using the same connection, which resulted in an ID conflict. I'm not certain how this happened because I'm completely unfamilar with your codebase, but it's somehow possible.
In my case, it was eth-block-tracker that was sending a message that somehow bypassed jsonrpc.js but still was being processed by the same websocket connection that jsonrpc.js was using, which resulted in a conflict.
Maybe someone more experienced than me could figure out how that happened? There might be another lurking bug there.
Description
Fixes #(issue)
Type of change
Checklist for 1.x:
npm run dtslintwith success and extended the tests and types if necessary.npm run test:covand my test cases cover all the lines and branches of the added code.npm run buildwith success.dist/web3.min.jsin a browser.CHANGELOG.mdfile in the root folder.Checklist for 4.x:
yarnsuccessfullyyarn lintsuccessfullyyarn build:websuccessfullyyarn test:unitsuccessfullyyarn test:integrationsuccessfullycompile:contractssuccessfullyCHANGELOG.mdfile in the packages I have edited.