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

Use FUNCTIONS_DISCOVERY_TIMEOUT when waiting for sockets #7838

Merged
merged 4 commits into from
Oct 16, 2024
Merged

Conversation

joehan
Copy link
Contributor

@joehan joehan commented Oct 15, 2024

Description

Fixed an issue during functions discovery where FUNCTIONS_DISCOVERY_TIMEOUT wasn't respected. Thanks @ErlikC for finding this fix!

Fixes #6285.

@joehan joehan requested a review from Berlioz October 15, 2024 21:16
Copy link
Contributor

@Berlioz Berlioz left a comment

Choose a reason for hiding this comment

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

The change looks good, but IME stuff which affects timeouts has a nasty tendency to break the integration test suites. Some of these failures look like flakes, but the very long duration of some of the failures has me worried.

LGTM if you can either get a green build or fix the tsts.

@joehan
Copy link
Contributor Author

joehan commented Oct 16, 2024

The change looks good, but IME stuff which affects timeouts has a nasty tendency to break the integration test suites. Some of these failures look like flakes, but the very long duration of some of the failures has me worried.

LGTM if you can either get a green build or fix the tsts.

Turns out these were actually catching a real bug in my code - I swapped a || for a ?? originally, which meant that when getFunctionDiscoveryTimeout returned a false but not undefined value, we'd set the timeout to 0 instead of the default value. Swapped it back and this seems to be good now

@joehan joehan enabled auto-merge (squash) October 16, 2024 20:37
@joehan joehan merged commit 9eed361 into master Oct 16, 2024
40 of 41 checks passed
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.

Failed to load function definition from source - functions emulator fails to start or deploy fails
2 participants