Correctly apply fix for high ulimit #2192

Earlopain · 2024-04-04T15:51:02Z

User description

Description

Commit c706217 changed the ulimit workaround so that it no longer works correctly:

-Sv modifies virtual memory, not file descriptors
The if check doesn't take the current ulimit into consideration, though it succeeds anyways

Motivation and Context

Should fix #2154. I myself am again able to connect to vnc.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

I have read the contributing document.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

Type

bug_fix

Description

Corrected the logic and command for updating the open file descriptor limit in both start-novnc.sh and start-vnc.sh scripts.
The scripts now check if the current limit is greater than a defined high limit or if a custom limit is specified, then attempt to update it.
Fixed the use of ulimit -n for setting the new limit, replacing the incorrect ulimit -Sv.
Improved feedback messages to accurately reflect the outcome of the limit update attempt.

Changes walkthrough

Relevant files

Bug fix

start-novnc.sh `Fix File Descriptor Limit Update in start-novnc.sh` NodeBase/start-novnc.sh Corrected the logic to update the open file descriptor limit based on a new or existing high limit. Fixed the command to change the limit (`ulimit -n`) and adjusted the condition to check the current limit. Improved the echo statements for success and failure of updating the limit.	+7/-5
start-vnc.sh `Correct File Descriptor Limit Adjustment in start-vnc.sh` NodeBase/start-vnc.sh Updated the logic for setting a new file descriptor limit if the current is too high or a custom limit is specified. Changed the `ulimit` command to correctly modify the file descriptor limit. Enhanced the output messages for clarity on success or failure of limit update.	+7/-5

✨ PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

Commit c706217 changed this logic so that it no longer works correctly: * `-SV` modified virtual memory, not file descriptors * The if check doesn't take the current ulimit into consideration, though it succeeds anyways

qodo-merge-pro · 2024-04-04T15:51:24Z

PR Description updated to latest commit (d7506b8)

Copy walkthrough table to "Files Changed" Tab

qodo-merge-pro · 2024-04-04T15:51:35Z

PR Review

⏱️ Estimated effort to review [1-5]	2, because the changes are straightforward and localized to two specific scripts, involving simple logic adjustments and command corrections. The context and purpose of the changes are clear, making it easier to review.
🧪 Relevant tests	No
🔍 Possible issues	Possible Bug: The condition `! -z "${SE_VNC_ULIMIT}"` checks if the variable is not empty but does not validate if the value is numeric or within a reasonable range. This could lead to unexpected behavior if an invalid value is provided.
🔍 Possible issues	Error Handling: The script prints a success message based on the exit status of `ulimit -n ${NEW_ULIMIT}` command. However, it does not explicitly handle or log cases where setting the new limit fails due to insufficient permissions or other system limitations.
🔒 Security concerns	No

✨ Review tool usage guide:

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

qodo-merge-pro · 2024-04-04T15:52:26Z

PR Code Suggestions

Category	Suggestions
Best practice	Use direct command execution within `if` to check for success. Instead of checking the exit status `$?` immediately after the `ulimit` command, consider using `if` directly to test the command's success. This approach is more concise and avoids the potential issue of `$?` being overwritten by commands executed between `ulimit` and the `if` statement. NodeBase/start-novnc.sh [15-16] -ulimit -n ${NEW_ULIMIT} -if [ $? -eq 0 ]; then +if ulimit -n ${NEW_ULIMIT}; then
Best practice	Validate `SE_VNC_ULIMIT` to ensure it's a numeric value before use. Consider validating the `SE_VNC_ULIMIT` environment variable before attempting to use it. This can prevent potential issues if the variable is set to a non-numeric value, which could cause the script to behave unexpectedly. NodeBase/start-novnc.sh [13] -NEW_ULIMIT=${SE_VNC_ULIMIT:-${TOO_HIGH_ULIMIT}} +if ! [[ "${SE_VNC_ULIMIT}" =~ ^[0-9]+$ ]]; then + echo "Warning: SE_VNC_ULIMIT is not a valid numeric value. Using default." + NEW_ULIMIT=${TOO_HIGH_ULIMIT} +else + NEW_ULIMIT=${SE_VNC_ULIMIT} +fi
Maintainability	Extract the `NEW_ULIMIT` calculation into a separate function for better readability. To improve readability and maintainability, consider extracting the logic for determining the `NEW_ULIMIT` into a separate function. This will make the main script flow cleaner and encapsulate the logic for determining the new limit in a single, reusable place. NodeBase/start-novnc.sh [11-13] -TOO_HIGH_ULIMIT=100000 -if [[ $(ulimit -n) -gt $TOO_HIGH_ULIMIT \|\| ! -z "${SE_VNC_ULIMIT}" ]]; then - NEW_ULIMIT=${SE_VNC_ULIMIT:-${TOO_HIGH_ULIMIT}} +calculate_new_ulimit() { + TOO_HIGH_ULIMIT=100000 + if [[ $(ulimit -n) -gt $TOO_HIGH_ULIMIT \|\| ! -z "${SE_VNC_ULIMIT}" ]]; then + echo ${SE_VNC_ULIMIT:-${TOO_HIGH_ULIMIT}} + fi +} +NEW_ULIMIT=$(calculate_new_ulimit)
Enhancement	Add error handling for the `ulimit` command to improve robustness. To ensure the script is robust against potential failures when setting the ulimit, consider adding a log message or handling mechanism in case the `ulimit` command fails. This can help in diagnosing issues in environments where the ulimit cannot be changed easily. NodeBase/start-vnc.sh [53-57] -ulimit -n ${NEW_ULIMIT} -if [ $? -eq 0 ]; then +if ! ulimit -n ${NEW_ULIMIT}; then + echo "Failed to update the open file descriptor limit. Current limit: $(ulimit -n)" +else echo "Successfully updated the open file descriptor limit." -else - echo "The open file descriptor limit could not be updated."

✨ Improve tool usage guide:

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

NodeBase/start-novnc.sh

VietND96 · 2024-04-04T19:55:18Z

Thank you @Earlopain for the explanation and fix

Correctly apply fix for high ulimit

d7506b8

Commit c706217 changed this logic so that it no longer works correctly: * `-SV` modified virtual memory, not file descriptors * The if check doesn't take the current ulimit into consideration, though it succeeds anyways

qodo-merge-pro bot added the bug_fix label Apr 4, 2024

qodo-merge-pro bot added the Review effort [1-5]: 2 label Apr 4, 2024

qodo-merge-pro bot reviewed Apr 4, 2024

View reviewed changes

NodeBase/start-novnc.sh Show resolved Hide resolved

VietND96 merged commit 9cbf328 into SeleniumHQ:trunk Apr 4, 2024
10 of 11 checks passed

Earlopain deleted the fix-novnc-hang branch April 4, 2024 19:58

b4handjr mentioned this pull request Apr 10, 2024

bug(nimbus): Fix for macos integration-tests mozilla/experimenter#10549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly apply fix for high ulimit #2192

Correctly apply fix for high ulimit #2192

Earlopain commented Apr 4, 2024 •

edited by qodo-merge-pro bot

Loading

qodo-merge-pro bot commented Apr 4, 2024 •

edited by VietND96

Loading

qodo-merge-pro bot commented Apr 4, 2024

qodo-merge-pro bot commented Apr 4, 2024

VietND96 commented Apr 4, 2024

Correctly apply fix for high ulimit #2192

Correctly apply fix for high ulimit #2192

Conversation

Earlopain commented Apr 4, 2024 • edited by qodo-merge-pro bot Loading

User description

Description

Motivation and Context

Types of changes

Checklist

Type

Description

Changes walkthrough

qodo-merge-pro bot commented Apr 4, 2024 • edited by VietND96 Loading

qodo-merge-pro bot commented Apr 4, 2024

PR Review

qodo-merge-pro bot commented Apr 4, 2024

PR Code Suggestions

VietND96 commented Apr 4, 2024

Earlopain commented Apr 4, 2024 •

edited by qodo-merge-pro bot

Loading

qodo-merge-pro bot commented Apr 4, 2024 •

edited by VietND96

Loading