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

Docker: Disable HeapDumpOnOutOfMemoryError by default #2708

Merged
merged 1 commit into from
Mar 17, 2025
Merged

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Mar 17, 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 #2706
It will be enabled when SE_JAVA_HEAP_DUMP is true

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, configuration changes


Description

  • Removed default enabling of HeapDumpOnOutOfMemoryError.

  • Added conditional handling of JAVA_OPTS in startup scripts.

  • Improved logging of JAVA_OPTS for better debugging.

  • Updated Dockerfile to reflect new default SE_JAVA_OPTS_DEFAULT.


Changes walkthrough 📝

Relevant files
Enhancement
10 files
start-selenium-grid-distributor.sh
Enhanced handling of `JAVA_OPTS` and heap dump configuration
+8/-5     
start-selenium-grid-eventbus.sh
Enhanced handling of `JAVA_OPTS` and heap dump configuration
+8/-5     
start-selenium-grid-hub.sh
Enhanced handling of `JAVA_OPTS` and heap dump configuration
+8/-5     
start-selenium-node.sh
Enhanced handling of `JAVA_OPTS` and heap dump configuration
+8/-5     
start-selenium-grid-docker.sh
Enhanced handling of `JAVA_OPTS` and heap dump configuration
+8/-5     
start-selenium-grid-router.sh
Enhanced handling of `JAVA_OPTS` and heap dump configuration
+8/-5     
start-selenium-grid-session-queue.sh
Enhanced handling of `JAVA_OPTS` and heap dump configuration
+8/-5     
start-selenium-grid-sessions.sh
Enhanced handling of `JAVA_OPTS` and heap dump configuration
+8/-5     
start-selenium-standalone.sh
Enhanced handling of `JAVA_OPTS` and heap dump configuration
+8/-5     
start-selenium-grid-docker.sh
Enhanced handling of `JAVA_OPTS` and heap dump configuration
+8/-5     
Configuration changes
1 files
Dockerfile
Removed default HeapDumpOnOutOfMemoryError from SE_JAVA_OPTS_DEFAULT
+1/-1     

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

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Variable Precedence

    The PR changes how JAVA_OPTS and SE_JAVA_OPTS interact. Previously, JAVA_OPTS took precedence over SE_JAVA_OPTS. Now SE_JAVA_OPTS is the base and JAVA_OPTS is appended to it. This could change behavior for existing deployments that rely on the previous precedence.

    if [ -n "${JAVA_OPTS}" ]; then
      SE_JAVA_OPTS="$SE_JAVA_OPTS ${JAVA_OPTS}"
    fi
    Default Configuration

    The PR removes the default HeapDumpOnOutOfMemoryError setting from SE_JAVA_OPTS_DEFAULT. This might affect error diagnostics in production environments where heap dumps were previously automatically generated on OOM errors.

    SE_JAVA_OPTS_DEFAULT="" \

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Remove redundant heap dump handling

    The handle_heap_dump function is now redundant since the
    HeapDumpOnOutOfMemoryError JVM option already specifies the heap dump path. The
    trap is unnecessary as the JVM will handle the heap dump automatically when an
    OutOfMemoryError occurs.

    Distributor/start-selenium-grid-distributor.sh [189-195]

    -function handle_heap_dump() {
    -  /opt/bin/handle_heap_dump.sh /opt/selenium/logs
    -}
     if [ "${SE_JAVA_HEAP_DUMP}" = "true" ]; then
       SE_JAVA_OPTS="$SE_JAVA_OPTS -XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=/opt/selenium/logs"
    -  trap handle_heap_dump ERR SIGTERM SIGINT
     fi
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies that the handle_heap_dump function and trap are redundant when the JVM option -XX:+HeapDumpOnOutOfMemoryError is set. The JVM will automatically create heap dumps at the specified path, making the manual trap handling unnecessary.

    Medium
    • More

    Copy link

    qodo-merge-pro bot commented Mar 17, 2025

    CI Feedback 🧐

    (Feedback updated until commit ff9d7e6)

    A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

    Action: Rerun workflow when failure

    Failed stage: Authenticate GitHub CLI for PR [❌]

    Failure summary:

    The action failed because the GitHub token provided in GH_CLI_TOKEN_PR is missing the required scope
    'read:org'. When attempting to authenticate with GitHub CLI using this token, the authentication
    failed with the error "missing required scope 'read:org'".

    Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    28:  SecurityEvents: write
    29:  Statuses: write
    30:  ##[endgroup]
    31:  Secret source: Actions
    32:  Prepare workflow directory
    33:  Prepare all required actions
    34:  Getting action download info
    35:  Download action repository 'actions/checkout@main' (SHA:85e6279cec87321a52edac9c87bce653a07cf6c2)
    36:  Complete job name: Rerun workflow when failure
    ...
    
    48:  show-progress: true
    49:  lfs: false
    50:  submodules: false
    51:  set-safe-directory: true
    52:  env:
    53:  GH_CLI_TOKEN: ***
    54:  GH_CLI_TOKEN_PR: ***
    55:  RUN_ID: 13891237527
    56:  RERUN_FAILED_ONLY: true
    ...
    
    119:  ##[group]Run sudo apt update
    120:  �[36;1msudo apt update�[0m
    121:  �[36;1msudo apt install gh�[0m
    122:  shell: /usr/bin/bash -e {0}
    123:  env:
    124:  GH_CLI_TOKEN: ***
    125:  GH_CLI_TOKEN_PR: ***
    126:  RUN_ID: 13891237527
    127:  RERUN_FAILED_ONLY: true
    ...
    
    181:  0 upgraded, 0 newly installed, 0 to remove and 50 not upgraded.
    182:  ##[group]Run echo "$GH_CLI_TOKEN_PR" | gh auth login --with-token
    183:  �[36;1mecho "$GH_CLI_TOKEN_PR" | gh auth login --with-token�[0m
    184:  shell: /usr/bin/bash -e {0}
    185:  env:
    186:  GH_CLI_TOKEN: ***
    187:  GH_CLI_TOKEN_PR: ***
    188:  RUN_ID: 13891237527
    189:  RERUN_FAILED_ONLY: true
    190:  RUN_ATTEMPT: 1
    191:  ##[endgroup]
    192:  error validating token: missing required scope 'read:org'
    193:  ##[error]Process completed with exit code 1.
    

    @VietND96 VietND96 merged commit 0c789ff into trunk Mar 17, 2025
    24 of 27 checks passed
    @VietND96 VietND96 deleted the java-opts branch March 17, 2025 05:09
    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]: HeapDumpOnOutOfMemoryError process consumes a lot of memory
    1 participant