search

status-im / nim-eth

Common utilities for Ethereum
https://nimbus.status.im
Apache License 2.0
82 stars 30 forks source link

Database backends not exception safe #160

Open arnetheduck opened 4 years ago

arnetheduck commented 4 years ago

All implementations of TrieDatabaseRef raise exceptions but have exception safety issues - in particular, resources will not be released / aborted / committed correctly leading to deadlocks / data loss.

Examples where a raise will leak or break - the code in general needs a full review, these are just samples:

https://github.com/status-im/nim-eth/blob/3ee5651b7c1b967e9aa115d99125ed35ac3ab2f1/eth/trie/backends/lmdb_backend.nim#L113 (txn not released) https://github.com/status-im/nim-eth/blob/3ee5651b7c1b967e9aa115d99125ed35ac3ab2f1/eth/trie/backends/sqlite_backend.nim#L57 (previous prepared statement not released) https://github.com/status-im/nim-rocksdb/blob/08fec021c0f28f63d1221d40a655078b5b923d1b/rocksdb.nim#L130 (leak partially opened db)

zah commented 4 years ago

The correct solution here is to use defer, finally or the upcoming Nim destructors. And I'm saying this only to disarm the potential argument that the problem here is the use of exceptions. Even with error codes, the clean-up paths are pretty hard to write correctly with regular control flow, especially in a language without goto. RAII is the best solution in both worlds.

arnetheduck commented 4 years ago

whatever it is, in practice it's not being done and has to be looked for manually now across the code base... good luck :)