Add -l argument to limit polling attempts to max 300

[SunMar]

When using npiperelay I found out that named pipes only allow a single client to connect to them at the same time (that might be a reason for preferring to use the Assuan socket if it worked). This was a problem because my IDE sometimes does multiple git/ssh commands in parallel, which would then error out.

The -p argument solved that, but I didn't like the idea of the relay doing attempts indefinitely, if for some reason a process ends up hanging without closing the pipe, this can create a build up of stuck npiperelay processes.

This PR adds a limit of 300 attempts (~60 seconds) for the -p flag. If it can't connect within those attempts, it will fail with the FILE_NOT_FOUND or ERROR_PIPE_BUSY error. Setting the limit to 300 is arbitrary, 60 seconds feels like a decent time to wait for a pipe to be available.

Alternatively, I can also introduce a new -l limit flag that optionally enables this failsafe to not impact existing behavior.

[albertony]

Thanks! Looks like a nice little improvement.

Alternatively, I can also introduce a new -l limit flag that optionally enables this failsafe to not impact existing behavior.

Hm.. I see arguments both ways, but I think I prefer this - i.e. adding it as an option.

[SunMar]

Sure, then I'll turn it into an optional flag. I'll do that after #52 is merged because I will need to modify the signature of dialPipe() and dialAssuan() to pass the flag.

[SunMar]

@albertony done, PR updated! this was actually my initial implementation so only had to push it

[albertony]

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? 🤓

[SunMar]

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 😖).

[albertony]

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).