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

feat: Add timeout option (fixes #15) #16

Merged
merged 7 commits into from
May 28, 2024
Merged

Conversation

rasa
Copy link
Contributor

@rasa rasa commented May 12, 2024

Based on your feedback, I made some assumptions:

  1. Default to minutes, not seconds.
  2. Use --timeout and -T, not duration

We use the ParseDuration function, which allows for:

A possibly signed sequence of decimal numbers, each with optional fraction and a unit suffix, such as "300ms", "-1.5h" or "2h45m". Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".

and allows for values up to 999,999 hours (114 years). This saved me time having to code a duration parser. I don't think we have to document this, though, do we?

@axllent
Copy link
Owner

axllent commented May 14, 2024

I agree with your approach, good call, but I'm having serious issues when trying to run this. Sorry, I really don't have much time to look at this at the moment, but I have given it a test and it has completely locked my system up (twice - requiring a forced restart). I believe there is something inherently wrong - I just don't know where exactly.

go run . test1 -T 1m appears to run fine until 1 minute is hit, after which everything grinds to a halt for a few seconds as it hits the timeout:

go run . test1 -T 1m
Calculating speed: 187,850 calculations per second using 11 CPU cores
Case-insensitive search, exiting after 1 result
Probability for "test1": 1 in 133,448,704 (approx 11 minutes per match)

Quitting after 1m0s, or sooner if all matching keys are found...

Press Ctrl-c to cancel

signal: killed

The signal: killed != fmt.Printf("Timed out after %v\n", c.timeout) - and the harddrive goes nuts, so I suspect it has something to do with the timeout logic. Are you not getting the same issue?

@rasa
Copy link
Contributor Author

rasa commented May 14, 2024

Are you not getting the same issue?

Gosh, I'm sorry you'd had to have dealt with that. I tested it on both a new Windows 11 64 AMD, and a five year old Ubuntu Nobel 64 AMD, without any issues with any timeouts.

The code seems pretty simple, but let me play around some more and see if I can figure out what's causing the issue.

Perhaps

ctx, _ := context.WithTimeout(context.Background(), c.timeout)

needs to be

ctx, cancel_func := context.WithTimeout(context.Background(), c.timeout)

and then we call cancel_func() when we're aborting.

@axllent
Copy link
Owner

axllent commented May 14, 2024

Maybe 🤷‍♂️ I really don't know, and I'd love to help you debug it but I'm somewhat overloaded at the moment (work/life).

@rasa
Copy link
Contributor Author

rasa commented May 24, 2024

@axllent I replaced Context with a simple NewTimer(), as that's all we really need. I tested in both Linux and Windows and it works fine. Lemme know if it works for you. If not, let's just drop it.

@axllent
Copy link
Owner

axllent commented May 24, 2024

@rasa I'm actually away for 3 days, so please don't think I'm ignoring you, but I'll only be able to test when I'm back next week 👍 Thanks for looking into it, looking forward to testing!

@axllent
Copy link
Owner

axllent commented May 27, 2024

@rasa - I've done some more testing and unfortunately it still crashes my system. I do however know now why it crashes, but just not what is causing it exactly. While running with any timeout set, the app's RAM usage increases +-1GB per second - so the machine effectively runs out of RAM.

Peek 2024-05-27 15-32

I am surprised you haven't encountered this issue though.

@rasa
Copy link
Contributor Author

rasa commented May 27, 2024

Wow, that's crazy. I tested it on Ubuntu and Windows, and no memory leaks. Can you try another system, or VM?

According to
https://blogtitle.github.io/go-advanced-concurrency-patterns-part-2-timers/
certain aspect of Timers are buggy. And I followed their advice in this PR, so I'm really stumped why you are experiencing issues, and I'm not.

@axllent
Copy link
Owner

axllent commented May 27, 2024

I've just tested on a completely different machine (a laptop also running Ubuntu 22.04) and it has the same issue, just a little slower (500MB/s increase, probably due to the smaller CPU / less threads). Completely different hardware (laptop / intel CPU vs desktop AMD). I don't think this is hardware related though, Go is pretty good that way.

Finally, I have just tested on an Ubuntu 24 VM (running in VirtualBox) and it also has the same RAM issue, so yeah, I believe it is an issue, I'm just surprised you don't have it (are you really sure though?). I'm monitoring it using htop (as seen in the gif I posted earlier) - when I run without the timeout option it is completely stable, but the moment I run it with the timeout option the RAM usage continually increases (fast). I'm also running your branch's code directly, ie: not a partial merge or anything.

Very odd...

@rasa
Copy link
Contributor Author

rasa commented May 27, 2024

Try it now. I misread your message, and only tested without a timer, which was just plain dumb in hindsight. With a timer active it leaked. With this latest fix, it doesn't. Sorry for all the extra work.

@axllent
Copy link
Owner

axllent commented May 27, 2024

Yes, that solves the issue, well done! I'll give this more testing in the coming day or two, but I can confirm there is no memory leak now. Thanks for finding that @rasa!

@axllent axllent merged commit 8f9d05a into axllent:develop May 28, 2024
@axllent
Copy link
Owner

axllent commented May 28, 2024

Works perfectly @rasa - thank you for you hard work. This has been released now as 0.1.0 🥳

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

Successfully merging this pull request may close these issues.

2 participants