core/txpool: change lock in Pending method of legacy pool to read lock

[pythonberg1997]

This PR makes a small update to the Pending() method in the legacy pool. By changing the lock from exclusive to read-only, it aims to improve concurrency performance.

[will-2012]

#31523. I tried this before, but it seems to have some issues. FYI.

[will-2012]

#31523. I tried this before, but it seems to have some issues. FYI.

SortedMap seems to have complex concurrent semantics. It seems that it is dangerous in concurrent situations.

Pls check and pay more attenttion :D.

[rjl493456442]

@will-2012 Thanks for pointing it out, you are right.

func TestSortedMapConcurrency(t *testing.T) {
	m := NewSortedMap()

	// Generate a list of transactions to insert
	key, _ := crypto.GenerateKey()
	txs := make(types.Transactions, 1000)
	for i := 0; i < len(txs); i++ {
		txs[i] = transaction(uint64(i), 0, key)
	}
	for i := 0; i < len(txs)/2; i++ {
		m.Put(txs[i])
	}
	
	var wg sync.WaitGroup
	wg.Add(8)
	for i := 0; i < 8; i++ {
		go func() {
			defer wg.Done()

			m.Flatten()
		}()
	}

	wg.Add(1)
	go func() {
		defer wg.Done()

		for i := len(txs) / 2; i < len(txs); i++ {
			m.Put(txs[i])
		}
	}()

	wg.Add(1)
	go func() {
		defer wg.Done()

		for i := 0; i < len(txs)/2; i++ {
			m.Forward(uint64(i))
		}
	}()

	wg.Wait()
}

I can reproduce the concurrent map read/write panic with this script.

[pythonberg1997]

@will-2012 Thanks for pointing it out, you are right.

func TestSortedMapConcurrency(t *testing.T) {
	m := NewSortedMap()

	// Generate a list of transactions to insert
	key, _ := crypto.GenerateKey()
	txs := make(types.Transactions, 1000)
	for i := 0; i < len(txs); i++ {
		txs[i] = transaction(uint64(i), 0, key)
	}
	for i := 0; i < len(txs)/2; i++ {
		m.Put(txs[i])
	}
	
	var wg sync.WaitGroup
	wg.Add(8)
	for i := 0; i < 8; i++ {
		go func() {
			defer wg.Done()

			m.Flatten()
		}()
	}

	wg.Add(1)
	go func() {
		defer wg.Done()

		for i := len(txs) / 2; i < len(txs); i++ {
			m.Put(txs[i])
		}
	}()

	wg.Add(1)
	go func() {
		defer wg.Done()

		for i := 0; i < len(txs)/2; i++ {
			m.Forward(uint64(i))
		}
	}()

	wg.Wait()
}

I can reproduce the concurrent map read/write panic with this script.

Cool. I see.

[buddh0]

I checked the code — both Content and Pending can safely use RLock.
The Put and Forward methods of SortedMap are already protected by Lock in their actual usage scenarios,
and Flatten is also guarded by RLock.
Therefore, the test case mentioned in the reply wouldn’t occur in practice.

@rjl493456442

[will-2012]

I checked the code — both Content and Pending can safely use RLock. The Put and Forward methods of SortedMap are already protected by Lock in their actual usage scenarios, and Flatten is also guarded by RLock. Therefore, the test case mentioned in the reply wouldn’t occur in practice.

@rjl493456442

Got it, SortedMap is not thread-safe, and the upper caller is responsible for concurrent access.