Skip to content
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

Fix simple-keyvalue-client crash #2542

Merged
merged 2 commits into from
Jan 13, 2020
Merged

Fix simple-keyvalue-client crash #2542

merged 2 commits into from
Jan 13, 2020

Conversation

kostko
Copy link
Member

@kostko kostko commented Jan 12, 2020

Fixes #2531

The crash reported in the above issue is actually result of two separate problems, both of which should be fixed by this PR:

  • The runtime client endpoint should return empty sequences instead of nil as serde doesn't know how to decode a NULL when the expected type is a sequence.
  • The simple-keyvalue client used a hardcoded round number in the query (3) which can easily be incorrect due to the time at which the client is called.

@kostko kostko requested a review from mitjat January 12, 2020 19:47
@codecov
Copy link

codecov bot commented Jan 12, 2020

Codecov Report

Merging #2542 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2542      +/-   ##
==========================================
+ Coverage   66.95%   66.96%   +0.01%     
==========================================
  Files         328      328              
  Lines       30184    30184              
==========================================
+ Hits        20210    20214       +4     
- Misses       7455     7462       +7     
+ Partials     2519     2508      -11
Impacted Files Coverage Δ
go/runtime/client/client.go 69.35% <100%> (ø) ⬆️
go/genesis/api/api.go 26.19% <0%> (-7.15%) ⬇️
go/worker/common/p2p/p2p.go 62.61% <0%> (-4.96%) ⬇️
go/consensus/tendermint/abci/mux.go 63.4% <0%> (-3.99%) ⬇️
go/oasis-node/cmd/node/node.go 53.8% <0%> (-3.34%) ⬇️
go/oasis-node/cmd/debug/storage/storage.go 44.44% <0%> (-3.34%) ⬇️
go/runtime/transaction/transaction.go 76.82% <0%> (-1.22%) ⬇️
go/storage/client/client.go 75.92% <0%> (-0.93%) ⬇️
go/consensus/tendermint/tendermint.go 62.5% <0%> (-0.87%) ⬇️
go/worker/common/committee/group.go 81.78% <0%> (-0.75%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83df083...71e63ec. Read the comment docs.

Copy link
Contributor

@mitjat mitjat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice finds! Thank you!

Sucks that golang didn't take a clue from Java about how painful it is to maintain nil-safety.

@kostko
Copy link
Member Author

kostko commented Jan 13, 2020

Sucks that golang didn't take a clue from Java about how painful it is to maintain nil-safety.

In this case the returned nil is not unsafe. The problem is that in Golang both a nil and an empty slice can be used interchangeably (e.g., in range constructs and append etc. builtins). But when serialized into CBOR a nil is encoded as CBOR NULL while an empty slice is encoded as an empty sequence.

However the Rust serialization library (serde) doesn't know how to deserialize a sequence (e.g. Vec<T>) from a NULL by default and decoding fails.

@kostko kostko merged commit d29e701 into master Jan 13, 2020
@kostko kostko deleted the kostko/fix/rt-cli-2531 branch January 13, 2020 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

simple-keyvalue-client crashes non-deterministally
3 participants