Skip to content

Add -l argument to limit polling attempts to max 300 #53

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 7, 2025
Merged

Add -l argument to limit polling attempts to max 300 #53

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions npiperelay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@ const (
cSECURITY_SQOS_PRESENT = 0x100000 //nolint:revive,stylecheck // Don't include revive and stylecheck when running golangci-lint to stop complain about use of underscores in Go names
cSECURITY_ANONYMOUS = 0 //nolint:revive,stylecheck // Don't include revive and stylecheck when running golangci-lint to stop complain about use of underscores in Go names
cPOLL_TIMEOUT = 200 * time.Millisecond //nolint:revive,stylecheck // Don't include revive and stylecheck when running golangci-lint to stop complain about use of underscores in Go names
cPOLL_ATTEMPTS = 300 //nolint:revive,stylecheck // Don't include revive and stylecheck when running golangci-lint to stop complain about use of underscores in Go names
)

var (
poll = flag.Bool("p", false, "poll until the the named pipe exists and is not busy")
poll = flag.Bool("p", false, "poll every 200ms until the the named pipe exists and is not busy")
limit = flag.Bool("l", false, "when polling do not poll indefinitely, fail after 300 attempts")
closeWrite = flag.Bool("s", false, "send a 0-byte message to the pipe after EOF on stdin")
closeOnEOF = flag.Bool("ep", false, "terminate on EOF reading from the pipe, even if there is more data to write")
closeOnStdinEOF = flag.Bool("ei", false, "terminate on EOF reading from stdin, even if there is more data to write")
Expand Down Expand Up @@ -59,17 +61,20 @@ func hideConsole() error {
return nil
}

func dialPipe(p string, poll bool) (*overlappedFile, error) {
func dialPipe(p string, poll bool, limit bool) (*overlappedFile, error) {
p16, err := windows.UTF16FromString(p)
if err != nil {
return nil, err
}
for {
for attempts := 0; ; {
Copy link
Owner

Choose a reason for hiding this comment

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

If one were to nitpick: One could argue that this is an off-by-one case. With cPOLL_ATTEMPTS 300 it may actually execute the windows.CreateFile 301 times. Changing to attempts := 1 would make it execute (at most) 300 times. 😝

Or is it... Because on the other hand, it will then have done the time.Sleep(cPOLL_TIMEOUT) 300 times, so it will have slept 60 seconds in total. 😞

If we had cPOLL_ATTEMPTS = 1 (maybe in future it is exposed as a flag, and a user set it to 1) then what is expected? 1 call to windows.CreateFile? Or 2? What is "a poll", or "one poll"? In non-polling mode you get 1 call to windows.CreateFile, so is the smallest possible poll mode 2 calls, hence a poll is basically a "retry"...? Or is that confusing, so 1 poll attempt equals 1 connection, 2 poll attempts is the initial connection attempt and 1 possible retry. Do you follow? Not a big deal by any possible means, mostly just a thought experiment from my side. But I'm leaning towards the latter interpretation being the least confusing, but curious to know what you think, and if you gave this a thought? 🤓

Copy link
Author

Choose a reason for hiding this comment

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

Whoops, you are completely right, it is an off-by-one case. I had considered calling the constant cPOLL_RETRIES but went for cPOLL_ATTEMPTS (and calling the flag as well limit rather than retries) with the idea that it would do 300 in total (including the initial CreateFile()), but with the implementation it will end up doing 301. It made sense for me to define the total of CreateFile() calls and not ignore the first iteration. So I was leaning the same direction as you.

You triggered me though to think more about it. With a sleep time of 200ms, it's so small that it doesn't really matter if it's doing X or X+1 attempts. But what if in the thought experiment we introduce allowing a user to specify cPOLL_TIMEOUT as well. Now you can increase the sleep time from 200ms to something way bigger, let's say 1 hour. What would be the expected behavior of cPOLL_ATTEMPTS in that use case? From a user perspective if I set the sleep time to 1 hour, I would like to be able to control the total time spent sleeping. I don't want to set the attempts to 3 and end up with a process that sleeps for 2 hours, that feels counter-intuitive (or rather I would prefer something like a max_sleep_time parameter over controlling the number of attempts), even though logically it is correct because in those 2 hours the application did do 3 total attempts.

Now I'm leaning more towards keeping the implementation like it is now, even though it's not what I originally had in mind. I have no strong preference either way though, since right now it's hard coded and the sleep time is so small, from a practical perspective it doesn't really matter which way the coin flips 😊.

Let me know what you prefer, will be happy to update the PR to a different approach, or just rename things if it adds better clarity, though at this moment I'm not sure what the best naming would be (naming things is always difficult 😖).

Copy link
Owner

Choose a reason for hiding this comment

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

From a user perspective if I set the sleep time to 1 hour, I would like to be able to control the total time spent sleeping.

That is a good point.

This just reminded me of the --retries option in rclone, which in reality means number of attempts. Setting --retries=2 means, if initial attempt fails, it will try once more. Setting --retries=1 means just try once, so effectively no retries. Setting --retries=0 is the same as --retries=1. I think that is more confusing than your limit option that controls polls.

As our existing option is called poll, and not retries or something like that, it is kind of vague already. And then limit fits that nicely. I think your take at it here is sound. Lets keep it!

Thank you (for your work, of course, but also for taking the time to discuss this topic, I appreciated it).

h, err := windows.CreateFile(&p16[0], windows.GENERIC_READ|windows.GENERIC_WRITE, 0, nil, windows.OPEN_EXISTING, windows.FILE_FLAG_OVERLAPPED|cSECURITY_SQOS_PRESENT|cSECURITY_ANONYMOUS, 0)
if err == nil {
return newOverlappedFile(h), nil
}
if poll {
if poll && attempts < cPOLL_ATTEMPTS {
if limit {
attempts++
}
if err == windows.ERROR_FILE_NOT_FOUND {
time.Sleep(cPOLL_TIMEOUT)
continue
Expand Down Expand Up @@ -117,8 +122,8 @@ func dialPort(p int, _ bool) (*overlappedFile, error) {
}

// LibAssaun file socket: Attempt to read contents of the target file and connect to a TCP port
func dialAssuan(p string, poll bool) (*overlappedFile, error) {
pipeConn, err := dialPipe(p, poll)
func dialAssuan(p string, poll bool, limit bool) (*overlappedFile, error) {
pipeConn, err := dialPipe(p, poll, limit)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -216,9 +221,9 @@ func main() {
var err error

if !*assuan {
conn, err = dialPipe(args[0], *poll)
conn, err = dialPipe(args[0], *poll, *limit)
} else {
conn, err = dialAssuan(args[0], *poll)
conn, err = dialAssuan(args[0], *poll, *limit)
}
if err != nil {
log.Fatalln(err)
Expand Down