remove unnecessary retries for locked port when there isn't a range t… #8020
Conversation
SwampDragons
force-pushed
the
fix_fd_leak_port_lock
branch
from
8388da2
to
246a17c
Compare
|
Okay, so I've re-read the flock code and to lock it opens a file to try & lock it; but does not close the file if it could not lock, so in our case, for every port in range ! And calling The right fix would have been this one: gofrs/flock#31 I will try to get that PR sorted out based on the codeowner's review |
|
Okay I added a test/refreshed that PR there gofrs/flock#43 |
| @@ -74,6 +74,19 @@ func (lc ListenRangeConfig) Listen(ctx context.Context) (*Listener, error) { | |||
|
|
|||
| err := retry.Config{ | |||
| RetryDelay: func() time.Duration { return 1 * time.Millisecond }, | |||
| ShouldRetry: func(err error) bool { | |||
There was a problem hiding this comment.
One simplification would be to justreturn portRange > 0.
Also if there is no port range may be we could use a linearly growing retry delay instead.
There was a problem hiding this comment.
Would there be a time when the port would come available if we waited long enough?
There was a problem hiding this comment.
oh... I guess when the previous build completes its boot command.
SwampDragons
force-pushed
the
fix_fd_leak_port_lock
branch
from
246a17c
to
c9c8a2e
Compare
|
@azr do we still need this now that you've merged a fix to the upstream library? |
|
I think we should keep retrying - event when Therefore I think we do not need that fix as it is fixed in master. Closing this one; please reopen if you disagree 🙂. |
|
I concur!! |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Last night, I started seeing a "too many open files" error in TestListenRangeConfig_Listen when I ran
make testlocally.I originally thought it was a side effect of a file descriptor leak, maybe from another test, that was recently created, but I did a bisect and it's failing for me all the way back to when this test and the port lock functionality were added in v1.4.1-dev. So something must have changed on my computer with my ulimit to make this test suddenly start failing -- however, even if it's my computer that changed recently, this looks an awful lot like a real file descriptor leak that I uncovered.
I think the leak is happening inside the retry function. Originally I tried just adding a lock.Close() here: https://github.com/hashicorp/packer/blob/master/common/net/configure_port.go#L93-L95 but that wasn't resolving the issue, and I'm not seeing where else a leak could be occurring in this code pathway.
For now, I've added a ShouldRetry func so we don't keep repeating the try if there are no other valid ports to attempt. @azr I'd appreciate your thoughts on this.
Closes #8035