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

Size and other calculation bugs in RingQueue #29

Open
vgough opened this issue Jun 10, 2024 · 1 comment
Open

Size and other calculation bugs in RingQueue #29

vgough opened this issue Jun 10, 2024 · 1 comment

Comments

@vgough
Copy link

vgough commented Jun 10, 2024

Size returns an invalid size (larger than the buffer) in some cases.

Due to #27 , I created a new Ring queue implementation: https://github.com/arg0net/collections

As part of it, I added a Fuzzing test to look for edge cases that I might not have considered. I tried running the fuzzer on Logdy's RingQueue out of curiosity since I was seeing strange behavior. What it found is that the RingQueue Size function is incorrect and can report a size greater than the size of the ring. Since Size is called from other functions, those functions are also incorrect.

Reproduction case:

  • create RingQueue of size N
  • queue N elements into it. The ring is now full.
  • Pop one element. now start is 1, and end is 0.
  • Size returns N+1, when it should return N-1.

Here's my hacked fix

func (r *RingQueue[T]) Size() int {
	res := r.end - r.start
	if res < 0 {
		res = len(r.data) + res
	} else if res == 0 && r.isFull {
		return len(r.data)
	}

	return res
}

After fixing that, the fuzzer next found a bug in PeekIdx. There's an off-by-one error which causes it to return a value for an empty ring. Since the ring values are not cleared, it is returning a previously used value from the queue. I believe the problem is that it isn't taking into account the isFull flag if start == end. I patched in a fix for that as well as the Size issue and then the fuzzer was still finding problems, so I've given up. I suggest implementing fuzzing to help track down the issues, or else consider importing the implementation I created. I'm happy to add additional methods if you need Scan or other methods.

FWIW, I also ran the benchmark on the arg0net and lodgy versions and found the arg0net is also 8x faster for repeating push/pops on a desktop server. I'm guessing that this is because there's no need to do modular arithmetic or the extra step of memory indexing (one is built into the array accessor) to determine an index.

Hope that helps!

@PeterOsinski
Copy link
Contributor

thanks, I'll look into that!

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

2 participants