Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

storage/localstore: add subscriptions wait group before closing leveldb #1980

Merged
merged 3 commits into from
Nov 26, 2019

Conversation

janos
Copy link
Member

@janos janos commented Nov 25, 2019

This PR add a protection against creating a LevelDB iterator after it has been closed. It is possible that a subscription goroutine can be scheduled after localstore DB Close method is called, resulting a panic:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x787fa8]
goroutine 3267 [running]:
github.com/syndtr/goleveldb/leveldb.(*DB).newRawIterator(0xc00025a340, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc0001149f0, 0x54)
    /home/travis/gopath/src/github.com/ethersphere/swarm/vendor/github.com/syndtr/goleveldb/leveldb/db_iter.go:54 +0x2a8
github.com/syndtr/goleveldb/leveldb.(*DB).newIterator(0xc00025a340, 0x0, 0x0, 0x0, 0x0, 0x54, 0x0, 0x0, 0x8b2a40)
    /home/travis/gopath/src/github.com/ethersphere/swarm/vendor/github.com/syndtr/goleveldb/leveldb/db_iter.go:79 +0x99
github.com/syndtr/goleveldb/leveldb.(*DB).NewIterator(0xc00025a340, 0x0, 0x0, 0x0, 0x0)
    /home/travis/gopath/src/github.com/ethersphere/swarm/vendor/github.com/syndtr/goleveldb/leveldb/db.go:891 +0x189
github.com/ethersphere/swarm/shed.(*DB).NewIterator(0xc000041680, 0xc000548001, 0x1)
    /home/travis/gopath/src/github.com/ethersphere/swarm/shed/db.go:140 +0xc0
github.com/ethersphere/swarm/shed.Index.Iterate(0xc000041680, 0xc00002c8e9, 0x1, 0x1, 0xc00304c640, 0xc0020c5800, 0x9210e8, 0x9210f0, 0xc0000c5d70, 0xc0000c5d08, ...)
    /home/travis/gopath/src/github.com/ethersphere/swarm/shed/index.go:304 +0x107
github.com/ethersphere/swarm/storage/localstore.(*DB).SubscribePull.func1(0x90aca5, 0x18, 0xc001095080, 0x0, 0xc000f32000, 0x5, 0xc0010950e0, 0x0, 0x99dca0, 0xc0007a0640, ...)
    /home/travis/gopath/src/github.com/ethersphere/swarm/storage/localstore/subscription_pull.go:90 +0x676
created by github.com/ethersphere/swarm/storage/localstore.(*DB).SubscribePull
    /home/travis/gopath/src/github.com/ethersphere/swarm/storage/localstore/subscription_pull.go:64 +0x3c4
FAIL    github.com/ethersphere/swarm/storage/localstore    31.341s

The issue was found by @acud.

@janos janos self-assigned this Nov 25, 2019
@janos janos added this to Backlog in Swarm Core - Sprint planning via automation Nov 25, 2019
@@ -432,6 +437,7 @@ func New(path string, baseKey []byte, o *Options) (db *DB, err error) {
func (db *DB) Close() (err error) {
close(db.close)
db.updateGCWG.Wait()
db.subscritionsWG.Wait()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not introduced by this PR only, but is it ok to do Wait() on an actual Close method, which is supposed to be for shutting down something? I mean, is it ensured that Wait() will not wait forever, e.g. because of blocked go routines?

Copy link
Member

Choose a reason for hiding this comment

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

well if you want a clean shutdown you'd probably want to wait until all of the goroutines that use a certain resource exit no?
if they don't exit it means that there's a deadlock and that should be fixed

Copy link
Member Author

Choose a reason for hiding this comment

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

As we talked, I've added a deadline of 5 seconds, that should be reasonable. And also I have added by @acud suggestion a goroutine profile dump, which we should improve int he future to control where it is written, currently it is in stdout.

@acud acud moved this from Backlog to In review (includes Documentation) in Swarm Core - Sprint planning Nov 26, 2019
@acud acud requested review from zelig and nolash November 26, 2019 05:06
@acud acud merged commit ebdd81b into master Nov 26, 2019
Swarm Core - Sprint planning automation moved this from In review (includes Documentation) to Done Nov 26, 2019
@acud acud deleted the localstore-subscriptions-wg branch November 26, 2019 14:37
@acud acud added this to the 0.5.5 milestone Jan 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants