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

Nested Store Prefix Iterators Interfere With Each Other #14786

Closed
SpicyLemon opened this issue Jan 25, 2023 · 23 comments
Closed

Nested Store Prefix Iterators Interfere With Each Other #14786

SpicyLemon opened this issue Jan 25, 2023 · 23 comments

Comments

@SpicyLemon
Copy link
Collaborator

Summary of Bug

Items from a nested store prefix iterator are added to other existing (still open and in use) store prefix iterators. This causes these other iterators to provide entries outside their intended range.

Version

v0.46.8

Steps to Reproduce

I found this because one of our unit tests started failing when we bumped our SDK to version v0.46.8. I'll try to describe it as succinctly as possible, but will also provide code at the bottom.

The function being testing named IterateRecordSpecsForOwner. It iterates over some index entries in state in order to get some specific "contract specification ids"; I will refer to this as the "outside iterator". For each contract specification id, it then iterates over the record specification's in state; I will refer to these ones as the "inside iterator(s)".

The iterators are created using sdk.KVStorePrefixIterator. The inside iterators are created, fully used, and closed while the outside iterator is still open and being used, i.e. the iterators are nested. The same store is provided for both, but different prefixes are provided that don't overlap.

There are three types of state entries involved in this unit test:

  1. An account address to contract specification id index. Keys: <type byte (32)><address length><address><contract specification id>.
  2. Contract specifications. Keys: <type byte (3)><contract spec id>. These aren't directly involved in the iteration, but are needed as part of setup.
  3. Record specifications. Keys: <type byte (5)><contract spec id><record spec name hash>.

The outer iterator is prefixed with <type byte (32)><address length><address>. It's .Key()s are expected to be <contract specification id>s.

The inside iterator is prefixed with <type byte (5)><contract spec id>. The entries it provides are expected to be record specifications.

The unit test that's failing has the following set up:

  1. Two account addresses: account 1, and account 2.
  2. Three contract specifications in state: contract spec 0 (associated with account 1), contract spec 1 (associated with account 2), and contract spec 2 (associated with both account 1 and 2).
  3. Six record specifications in state: two for each contract specification.
  4. Four index entries: account 1 -> contract spec 0, account 1 -> contract spec 2, account 2 -> contract spec 1, account 2 -> contract spec 2.

After setup, IterateRecordSpecsForOwner is called for account 1. It is expected to iterate over four record specs: two each from contract specs 0 and 2. Instead, the second value provided by the outer iterator is a record spec entry (which it fails to decode since it's expecting only index entries).

In IntelliJ, i used the debugger on the unit test in question to investigate.

I verified that the store has everything expected, i.e. store.parent.cache = {map[string]*cachekv.cValue} has all twelve items (store = {types.KVStore | *gaskv.Store} , and store.parent = {types.KVStore | *cachekv.Store}).

Here's what I'm seeing as it runs:

  1. Outside iterator created.
  2. .Valid() called on outside iterator: returns true.
  3. .Key() called on outside iterator: returns the first index key as expected (account 1 -> contract spec 0).
    Here's what the debugger had to say about the outside iterator:
    it = {db.Iterator | *gaskv.gasIterator} 
     gasMeter = {types.GasMeter | *types.infiniteGasMeter} InfiniteGasMeter:\n  consumed: 104913
     gasConfig = {types.GasConfig}
     parent = {db.Iterator | *internal.cacheMergeIterator}
      parent = {db.Iterator | *iavl.UnsavedFastIterator}
      cache = {db.Iterator | *internal.memIterator}
       iter = {btree.GenericIter[internal.item]}
        tr = {*btree.BTreeG[internal.item] | 0x14000e32380}
         cow = {uint64} 178
         mu = {*sync.RWMutex | 0x140005ae408}
         root = {*btree.node[internal.item] | 0x14001cded80}
          cow = {uint64} 178
          count = {int} 2
          items = {[]internal.item} len:2, cap:2
           0 = {internal.item}
            key = {[]uint8} len:39, cap:48
            value = {[]uint8} len:1, cap:1
           1 = {internal.item}
            key = {[]uint8} len:39, cap:48
            value = {[]uint8} len:1, cap:1
          children = {*[]*btree.node[internal.item] | 0x0} nil
         count = {int} 2
    
    Not shown here is that it.parent.parent = {db.Iterator | *iavl.UnsavedFastIterator} .Valid() is false, but it.parent.cache .Valid() is true.
    Also not included is that,it.parent.cache.iter.stack[0]: .i = 0 and .n.items has 2 entries: the two index entries expected.
  4. An inside iterator is created for the first contract specification id.
  5. The inside iterator steps through its two entries and is then invalid and closed.
  6. .Next() called on outside iterator.
  7. .Valid() called on outside iterator: returns true.
  8. .Key() called on outside iterator: returns the key for the second record specification that was seen in step 5.
    Here's what the debugger had to say about the outside iterator now:
    it = {db.Iterator | *gaskv.gasIterator} 
     gasMeter = {types.GasMeter | *types.infiniteGasMeter} InfiniteGasMeter:\n  consumed: 106539
     gasConfig = {types.GasConfig}
     parent = {db.Iterator | *internal.cacheMergeIterator}
      parent = {db.Iterator | *iavl.UnsavedFastIterator}
      cache = {db.Iterator | *internal.memIterator}
       iter = {btree.GenericIter[internal.item]}
        tr = {*btree.BTreeG[internal.item] | 0x14000e32380}
         cow = {uint64} 178
         mu = {*sync.RWMutex | 0x140005ae408}
         root = {*btree.node[internal.item] | 0x14001cded80}
          cow = {uint64} 178
          count = {int} 4
          items = {[]internal.item} len:4, cap:4
           0 = {internal.item}
            key = {[]uint8} len:33, cap:48
            value = {[]uint8} len:121, cap:121
           1 = {internal.item}
            key = {[]uint8} len:33, cap:48
            value = {[]uint8} len:121, cap:121
           2 = {internal.item}
            key = {[]uint8} len:39, cap:48
            value = {[]uint8} len:1, cap:1
           3 = {internal.item}
            key = {[]uint8} len:39, cap:48
            value = {[]uint8} len:1, cap:1
          children = {*[]*btree.node[internal.item] | 0x0} nil
         count = {int} 4
    
    Here again, it.parent.parent .Valid() is false and it.parent.cache .Valid() is true.
    Also not included in that,it.parent.cache.iter.stack[0]: .i = 1, .n.items has 4 entries: the two record specs seen in step 5, then the two index entries.
    The it.parent.cache.iter.tr.root.items entries are [0] = the first record spec seen by the inside iterator, [1] = the second record spec seen, [2] = the first index entry seen, [3] = the original second index entry (this is what the iterator is expected to be on right now).
    At this point, store.parent.unsortedCache = {map[string]struct{}} has eight items, and store.parent.sortedCache = {*internal.BTree | 0x14000fec4d0} has the four items that the outside iterator now has.
  9. Test fails due to an unexpected .Key() from the outside iterator.

Code

The unit test: TestIterateRecordSpecsForOwner
All assertions in that test fail, the first of which is on line 396.

IterateRecordSpecsForOwner:

// IterateRecordSpecsForOwner processes all record specs owned by an address using a given handler.
func (k Keeper) IterateRecordSpecsForOwner(ctx sdk.Context, ownerAddress sdk.AccAddress, handler func(recordSpecID types.MetadataAddress) (stop bool)) error {
	var recordItErr error
	contractItErr := k.IterateContractSpecsForOwner(ctx, ownerAddress, func(contractSpecID types.MetadataAddress) bool {
		needToStop := false
		recordItErr = k.IterateRecordSpecsForContractSpec(ctx, contractSpecID, func(recordSpecID types.MetadataAddress) bool {
			needToStop = handler(recordSpecID)
			return needToStop
		})
		if recordItErr != nil {
			return true
		}
		return needToStop
	})
	if recordItErr != nil {
		return recordItErr
	}
	if contractItErr != nil {
		return contractItErr
	}
	return nil
}

IterateContractSpecsForOwner:

// IterateContractSpecsForOwner processes all contract specs owned by an address using a given handler.
func (k Keeper) IterateContractSpecsForOwner(ctx sdk.Context, ownerAddress sdk.AccAddress, handler func(contractSpecID types.MetadataAddress) (stop bool)) error {
	store := ctx.KVStore(k.storeKey)
	prefix := types.GetAddressContractSpecCacheIteratorPrefix(ownerAddress)
	it := sdk.KVStorePrefixIterator(store, prefix)
	defer it.Close()
	for ; it.Valid(); it.Next() {
		var contractSpecID types.MetadataAddress
		if err := contractSpecID.Unmarshal(it.Key()[len(prefix):]); err != nil {
			return err
		}
		if handler(contractSpecID) {
			break
		}
	}
	return nil
}

IterateRecordSpecsForContractSpec:

// IterateRecordSpecsForContractSpec processes all record specs for a contract spec using a given handler.
func (k Keeper) IterateRecordSpecsForContractSpec(ctx sdk.Context, contractSpecID types.MetadataAddress, handler func(recordSpecID types.MetadataAddress) (stop bool)) error {
	store := ctx.KVStore(k.storeKey)
	prefix, err := contractSpecID.ContractSpecRecordSpecIteratorPrefix()
	if err != nil {
		return err
	}
	it := sdk.KVStorePrefixIterator(store, prefix)
	defer it.Close()
	for ; it.Valid(); it.Next() {
		var recordSpecID types.MetadataAddress
		if err := recordSpecID.Unmarshal(it.Key()); err != nil {
			return err
		}
		if handler(recordSpecID) {
			break
		}
	}
	return nil
}
@alexanderbez
Copy link
Contributor

So I'm pretty sure nested iterators are not supported by IAVL/tm-db in the SDK. I'm not sure if there's explicit documentation of this? I do know ICS used nested iterators in various places which causes many issues, once they were removed, the issues resolved.

I'm also pretty confident you won't find any examples of them in the SDK's core modules, but I could be wrong.

That being said, it seems like a case we should support.

@SpicyLemon
Copy link
Collaborator Author

So I'm pretty sure nested iterators are not supported by IAVL/tm-db in the SDK. I'm not sure if there's explicit documentation of this? I do know ICS used nested iterators in various places which causes many issues, once they were removed, the issues resolved.

I'm also pretty confident you won't find any examples of them in the SDK's core modules, but I could be wrong.

That being said, it seems like a case we should support.

I couldn't find anything specific in documentation either. It's something that worked just fine on v0.46.7 (and before), though.

@alexanderbez
Copy link
Contributor

Interesting. So you're suggesting #14349 could be the culprit?

ICS had this issue I'm pretty sure way before that PR was merged though. Maybe your tests just didn't expose it?

cc @mpoke does this look familiar?

@SpicyLemon
Copy link
Collaborator Author

Interesting. So you're suggesting #14349 could be the culprit?

In our cosmos fork, I reverted that PR and pulled the result into the provenance repo, and the test passed again.

It's probably worth noting here that during the run of both iterators discussed in the bug report, there are only store reads being done (no store writes).

@tac0turtle
Copy link
Member

there was a testcase added that fixed the issue present before, there were also more reports of the same issue. We should aim to keep that test passing and see how to modify to have tests pass that replicate the provanence design

@08d2
Copy link
Contributor

08d2 commented Jan 26, 2023

There are three types of state entries involved in this unit test:

  • An account address to contract specification id index. Keys: <type byte (32)>.
  • Contract specifications. Keys: <type byte (3)>. These aren't directly involved in the iteration, but are needed as part of setup.
  • Record specifications. Keys: <type byte (5)>.

Are all of these keys used with the same store? Your code uses the same StoreKey, so it appears so? Unless your module ensures that the first bytes of each of those type bytes never overlap with any other type byte definitions, this doesn't work. There's no way for iterators, or prefix stores, to distinguish between keys with the same prefix.

@yihuang
Copy link
Collaborator

yihuang commented Jan 26, 2023

Probably not related to this issue, I just find that you may need to do cloneAppend in cases like this, there's a chance append reallocate the slice: https://github.com/provenance-io/provenance/blob/4421e1f6ff1f0f757b62f829b3c7b85c148164e6/x/metadata/types/address.go#L169

@yihuang
Copy link
Collaborator

yihuang commented Jan 26, 2023

func TestIteratorNested(t *testing.T) {
	mem := dbadapter.Store{DB: dbm.NewMemDB()}
	store := cachekv.NewStore(mem)

	// setup:
	// - (owner, contract id) -> contract
	// - (contract id, record id) -> record
	owner1 := 1
	contract1 := 1
	contract2 := 2
	record1 := 1
	record2 := 2
	store.Set([]byte(fmt.Sprintf("c%04d%04d", owner1, contract1)), []byte("contract1"))
	store.Set([]byte(fmt.Sprintf("c%04d%04d", owner1, contract2)), []byte("contract2"))
	store.Set([]byte(fmt.Sprintf("r%04d%04d", contract1, record1)), []byte("contract1-record1"))
	store.Set([]byte(fmt.Sprintf("r%04d%04d", contract1, record2)), []byte("contract1-record2"))
	store.Set([]byte(fmt.Sprintf("r%04d%04d", contract2, record1)), []byte("contract2-record1"))
	store.Set([]byte(fmt.Sprintf("r%04d%04d", contract2, record2)), []byte("contract2-record2"))

	it := types.KVStorePrefixIterator(store, []byte(fmt.Sprintf("c%04d", owner1)))
	defer it.Close()

	var records []string
	for ; it.Valid(); it.Next() {
		contractID, err := strconv.ParseInt(string(it.Key()[5:]), 10, 32)
		require.NoError(t, err)

		it2 := types.KVStorePrefixIterator(store, []byte(fmt.Sprintf("r%04d", contractID)))
		for ; it2.Valid(); it2.Next() {
			records = append(records, string(it2.Value()))
		}

		it2.Close()
	}

	require.Equal(t, []string{
		"contract1-record1",
		"contract1-record2",
		"contract2-record1",
		"contract2-record2",
	}, records)
}

I wrote a similar test case according to the description, but it succeeds, is there difference in the case, or ideally can you write a test case that reproduce in sdk? @SpicyLemon

@SpicyLemon
Copy link
Collaborator Author

There are three types of state entries involved in this unit test:

  • An account address to contract specification id index. Keys: <type byte (32)>.
  • Contract specifications. Keys: <type byte (3)>. These aren't directly involved in the iteration, but are needed as part of setup.
  • Record specifications. Keys: <type byte (5)>.

Are all of these keys used with the same store? Your code uses the same StoreKey, so it appears so? Unless your module ensures that the first bytes of each of those type bytes never overlap with any other type byte definitions, this doesn't work. There's no way for iterators, or prefix stores, to distinguish between keys with the same prefix.

Yes. The number in parentheses there is the int value used for each type. I.e. <type byte (32)> => 0x20, <type byte (5)> => 0x05, and <type byte (5)> => 0x05.

@SpicyLemon
Copy link
Collaborator Author

Probably not related to this issue, I just find that you may need to do cloneAppend in cases like this, there's a chance append reallocate the slice: https://github.com/provenance-io/provenance/blob/4421e1f6ff1f0f757b62f829b3c7b85c148164e6/x/metadata/types/address.go#L169

Yeah. Most of our keys stuff should clone and append instead of append. We haven't addressed it yet, though, because in all our use cases, the result is longer than the default capacity, so it doesn't alter the original prefix slices.

@SpicyLemon
Copy link
Collaborator Author

I wrote a similar test case according to the description, but it succeeds, is there difference in the case, or ideally can you write a test case that reproduce in sdk? @SpicyLemon

Yeah. I'll see if I can. Maybe there's something about using a different instance of the store and having it drop out of scope.

@08d2
Copy link
Contributor

08d2 commented Jan 26, 2023

There are three types of state entries involved in this unit test:

  • An account address to contract specification id index. Keys: <type byte (32)>.
  • Contract specifications. Keys: <type byte (3)>. These aren't directly involved in the iteration, but are needed as part of setup.
  • Record specifications. Keys: <type byte (5)>.

Are all of these keys used with the same store? Your code uses the same StoreKey, so it appears so? Unless your module ensures that the first bytes of each of those type bytes never overlap with any other type byte definitions, this doesn't work. There's no way for iterators, or prefix stores, to distinguish between keys with the same prefix.

Yes. The number in parentheses there is the int value used for each type. I.e. <type byte (32)> => 0x20, <type byte (5)> => 0x05, and <type byte (5)> => 0x05.

Oh! Sorry, I thought the numbers meant byte width, not byte value.

@SpicyLemon
Copy link
Collaborator Author

SpicyLemon commented Jan 26, 2023

I wrote a similar test case according to the description, but it succeeds, is there difference in the case, or ideally can you write a test case that reproduce in sdk? @SpicyLemon

After diving into rabbit holes for a few hours, I finally got a test to fail:

func TestIteratorNested(t *testing.T) {
	mem := dbadapter.Store{DB: dbm.NewMemDB()}
	store := cachekv.NewStore(mem)

	// setup:
	// - (owner, contract id) -> contract
	// - (contract id, record id) -> record
	owner1 := 1
	contract1 := 1
	contract2 := 2
	record1 := 1
	record2 := 2
	store.Set([]byte(fmt.Sprintf("c%04d%04d", owner1, contract1)), []byte("contract1"))
	store.Set([]byte(fmt.Sprintf("c%04d%04d", owner1, contract2)), []byte("contract2"))
	store.Set([]byte(fmt.Sprintf("R%04d%04d", contract1, record1)), []byte("contract1-record1"))
	store.Set([]byte(fmt.Sprintf("R%04d%04d", contract1, record2)), []byte("contract1-record2"))
	store.Set([]byte(fmt.Sprintf("R%04d%04d", contract2, record1)), []byte("contract2-record1"))
	store.Set([]byte(fmt.Sprintf("R%04d%04d", contract2, record2)), []byte("contract2-record2"))

	it := types.KVStorePrefixIterator(store, []byte(fmt.Sprintf("c%04d", owner1)))
	defer it.Close()

	var records []string
	for ; it.Valid(); it.Next() {
		contractID, err := strconv.ParseInt(string(it.Key()[5:]), 10, 32)
		require.NoError(t, err)

		it2 := types.KVStorePrefixIterator(store, []byte(fmt.Sprintf("R%04d", contractID)))
		for ; it2.Valid(); it2.Next() {
			records = append(records, string(it2.Value()))
		}

		it2.Close()
	}

	require.Equal(t, []string{
		"contract1-record1",
		"contract1-record2",
		"contract2-record1",
		"contract2-record2",
	}, records)
}

The only difference between this one and the one you shared earlier, is that I switched the r to an R for the first byte of the record keys. That way, the record entries sort smaller than the index entries.

Before making that switch, I looked at the debugger and saw that the record entries were still being added to the index iterator. The reason it wasn't a problem is that they were greater than the iterator's end key.

Here's what the failure looks like:

=== RUN   TestIteratorNested
    store_test.go:724: 
        	Error Trace:	/Users/danielwedul/git/cosmos-sdk/store/cachekv/store_test.go:724
        	Error:      	Not equal: 
        	            	expected: []string{"contract1-record1", "contract1-record2", "contract2-record1", "contract2-record2"}
        	            	actual  : []string{"contract1-record1", "contract1-record2", "contract2-record1", "contract2-record2", "contract1-record1", "contract1-record2", "contract2-record1", "contract2-record2", "contract1-record1", "contract1-record2", "contract2-record1", "contract2-record2"}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,2 +1,10 @@
        	            	-([]string) (len=4) {
        	            	+([]string) (len=12) {
        	            	+ (string) (len=17) "contract1-record1",
        	            	+ (string) (len=17) "contract1-record2",
        	            	+ (string) (len=17) "contract2-record1",
        	            	+ (string) (len=17) "contract2-record2",
        	            	+ (string) (len=17) "contract1-record1",
        	            	+ (string) (len=17) "contract1-record2",
        	            	+ (string) (len=17) "contract2-record1",
        	            	+ (string) (len=17) "contract2-record2",
        	            	  (string) (len=17) "contract1-record1",
        	Test:       	TestIteratorNested
--- FAIL: TestIteratorNested (0.00s)


Expected :[]string{"contract1-record1", "contract1-record2", "contract2-record1", "contract2-record2"}
Actual   :[]string{"contract1-record1", "contract1-record2", "contract2-record1", "contract2-record2", "contract1-record1", "contract1-record2", "contract2-record1", "contract2-record2", "contract1-record1", "contract1-record2", "contract2-record1", "contract2 ...

It could probably be tweaked/clarified to assert that the index iterator (it) is returning the two expected values.

The first time through, it.Key() returns c00010001. The second time, it returns R00010002. It misses the R there and erroneously extracts the "contractID" of 2 and iterates over those records. All total, it returns these keys: "c00010001", "R00010002", "R00020001", "R00020002", "c00010001", "c00010002".

@SpicyLemon
Copy link
Collaborator Author

SpicyLemon commented Jan 26, 2023

I added that test to three branches:

  • dwedul/14786-iter-test-v0.46.8 diff: based off of v0.46.8, using go 1.18, the test fails.
  • dwedul/14786-iter-test-v0.47.x diff: based off of release/v0.47.x, using go 1.19, the test fails.
  • dwedul/14786-iter-test-main diff: based off of main at commit 938143f03, using go 1.19, the test passes.

Feel free to update/use those branches for whatever.

@SpicyLemon
Copy link
Collaborator Author

I tweaked the test a bit (on each branch) to keep track of the keys that the index iterator (it) goes through, and assert that they're as expected.

The failure message now looks like this:

=== RUN   TestIteratorNested
    store_test.go:727: 
        	Error Trace:	/Users/danielwedul/git/cosmos-sdk/store/cachekv/store_test.go:727
        	Error:      	Not equal: 
        	            	expected: []string{"c00010001", "c00010002"}
        	            	actual  : []string{"c00010001", "R00010002", "R00020001", "R00020002", "c00010001", "c00010002"}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,2 +1,6 @@
        	            	-([]string) (len=2) {
        	            	+([]string) (len=6) {
        	            	+ (string) (len=9) "c00010001",
        	            	+ (string) (len=9) "R00010002",
        	            	+ (string) (len=9) "R00020001",
        	            	+ (string) (len=9) "R00020002",
        	            	  (string) (len=9) "c00010001",
        	Test:       	TestIteratorNested
        	Messages:   	indexes iterated
    store_test.go:732: 
        	Error Trace:	/Users/danielwedul/git/cosmos-sdk/store/cachekv/store_test.go:732
        	Error:      	Not equal: 
        	            	expected: []string{"contract1-record1", "contract1-record2", "contract2-record1", "contract2-record2"}
        	            	actual  : []string{"contract1-record1", "contract1-record2", "contract2-record1", "contract2-record2", "contract1-record1", "contract1-record2", "contract2-record1", "contract2-record2", "contract1-record1", "contract1-record2", "contract2-record1", "contract2-record2"}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,2 +1,10 @@
        	            	-([]string) (len=4) {
        	            	+([]string) (len=12) {
        	            	+ (string) (len=17) "contract1-record1",
        	            	+ (string) (len=17) "contract1-record2",
        	            	+ (string) (len=17) "contract2-record1",
        	            	+ (string) (len=17) "contract2-record2",
        	            	+ (string) (len=17) "contract1-record1",
        	            	+ (string) (len=17) "contract1-record2",
        	            	+ (string) (len=17) "contract2-record1",
        	            	+ (string) (len=17) "contract2-record2",
        	            	  (string) (len=17) "contract1-record1",
        	Test:       	TestIteratorNested
        	Messages:   	records iterated
--- FAIL: TestIteratorNested (0.00s)

yihuang added a commit to yihuang/cosmos-sdk that referenced this issue Jan 27, 2023
Closes: cosmos#14786
Solution:
- do a copy on btree before iteration
@yihuang
Copy link
Collaborator

yihuang commented Jan 27, 2023

fixed, thanks for your priceless test case, @SpicyLemon

@tac0turtle
Copy link
Member

Thank you to both of you for the tremendous help here!!!

@mpoke
Copy link

mpoke commented Jan 27, 2023

Interesting. So you're suggesting #14349 could be the culprit?

ICS had this issue I'm pretty sure way before that PR was merged though. Maybe your tests just didn't expose it?

cc @mpoke does this look familiar?

@alexanderbez It does look painfully familiar :) It's hard to tell if it's the same problem as we had nested iterators all over our implementation. We just decided to avoid doing that. Now we are much more conservative with our use of iterators.

@SpicyLemon
Copy link
Collaborator Author

I don't see any PRs associated with this. How was it completed?

@alexanderbez
Copy link
Contributor

I don't see any PRs associated with this. How was it completed?

#14798

@tac0turtle
Copy link
Member

I thought the pr was linked. Sorry for not sending it your way. Let us know if you have any feedback and we can incorporate

@SpicyLemon
Copy link
Collaborator Author

I thought the pr was linked. Sorry for not sending it your way. Let us know if you have any feedback and we can incorporate

Yay for Github! My best guess is that Github's auto-linking/closing parsing stuff is case sensitive, looking for all-lowercase "closes" (ignoring "Closes"). All good now though!

Thanks!

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

No branches or pull requests

6 participants