Skip to content

Docker: Add Node env var SE_NODE_CONNECTION_LIMIT_PER_SESSION and README #2855

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

Merged
merged 1 commit into from
Jun 6, 2025

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jun 6, 2025

User description

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Fixes #2850

Motivation and Context

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.

PR Type

enhancement, documentation


Description

  • Introduced SE_NODE_CONNECTION_LIMIT_PER_SESSION environment variable.

    • Added to Node Dockerfile with default value.
    • Documented in ENV_VARIABLES.md and README.md.
    • Included in environment variable generation scripts.
  • Updated documentation to address WebSocket connection exhaustion.

    • Provided troubleshooting and configuration guidance for connection limits.

Changes walkthrough 📝

Relevant files
Enhancement
Dockerfile
Add SE_NODE_CONNECTION_LIMIT_PER_SESSION to Node Dockerfile

NodeBase/Dockerfile

  • Added SE_NODE_CONNECTION_LIMIT_PER_SESSION with default value 10 to
    Node environment variables.
  • +1/-0     
    description.yaml
    Add descriptions for new environment variables                     

    scripts/generate_list_env_vars/description.yaml

  • Added descriptions for SE_EXTRA_LIBS,
    SE_NODE_CONNECTION_LIMIT_PER_SESSION, and
    SE_SUPERVISORD_UNIX_SERVER_PASSWORD.
  • +9/-0     
    value.yaml
    Add default values for new environment variables                 

    scripts/generate_list_env_vars/value.yaml

  • Added default values for SE_EXTRA_LIBS,
    SE_NODE_CONNECTION_LIMIT_PER_SESSION, and
    SE_SUPERVISORD_UNIX_SERVER_PASSWORD.
  • Set default for SE_NODE_CONNECTION_LIMIT_PER_SESSION to 200.
  • +6/-0     
    Documentation
    ENV_VARIABLES.md
    Document new and updated environment variables                     

    ENV_VARIABLES.md

  • Documented SE_NODE_CONNECTION_LIMIT_PER_SESSION and
    SE_SUPERVISORD_UNIX_SERVER_PASSWORD.
  • Clarified SE_EXTRA_LIBS description.
  • +3/-1     
    README.md
    Add troubleshooting and usage for connection limit per session

    README.md

  • Added troubleshooting section for WebSocket connection exhaustion.
  • Explained usage and adjustment of
    SE_NODE_CONNECTION_LIMIT_PER_SESSION.
  • Linked to related issue and described default behavior.
  • +9/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • …EADME
    
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    Copy link

    qodo-merge-pro bot commented Jun 6, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    2850 - PR Code Verified

    Compliant requirements:

    • Fix ConnectionFailedException: JdkWebSocket initial request execution error occurring in Selenium Grid

    Requires further human verification:

    • Verify the fix works with versions 4.26.0-20241101 and later (up to 4.33.0-20250525)
    • Confirm that CDP features work properly with the new configuration

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Default Value Inconsistency

    The default value for SE_NODE_CONNECTION_LIMIT_PER_SESSION is set to 200 in the value.yaml file, but in the NodeBase/Dockerfile and README.md it's set to 10. This inconsistency could lead to confusion or unexpected behavior.

    - name: SE_NODE_CONNECTION_LIMIT_PER_SESSION
      default: '200'

    Copy link

    qodo-merge-pro bot commented Jun 6, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Align inconsistent default values

    The default value for SE_NODE_CONNECTION_LIMIT_PER_SESSION in the Dockerfile
    (10) is inconsistent with the value in value.yaml (200). This inconsistency
    could cause confusion and unexpected behavior. Align the default value with
    what's defined in the configuration files.

    NodeBase/Dockerfile [52]

    -SE_NODE_CONNECTION_LIMIT_PER_SESSION="10" \
    +SE_NODE_CONNECTION_LIMIT_PER_SESSION="200" \
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies a real inconsistency between the default values in the Dockerfile (10) and value.yaml (200). However, based on the README.md context mentioning the default is 10, the fix direction may be incorrect - value.yaml should likely be changed to 10 instead.

    Medium
    • More

    @VietND96 VietND96 merged commit dbdfc1f into trunk Jun 6, 2025
    28 checks passed
    @VietND96 VietND96 deleted the node-ws-limit branch June 6, 2025 18:39
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: ConnectionFailedException: JdkWebSocket initial request execution error
    1 participant