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

[grid][java] fix node-docker #13789

Merged
merged 4 commits into from
Apr 10, 2024
Merged

[grid][java] fix node-docker #13789

merged 4 commits into from
Apr 10, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Apr 8, 2024

User description

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

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

Description

Motivation and Context

Fix: SeleniumHQ/docker-selenium#2175
RCA: regression issue belongs to #13647

19:22:13.932 DEBUG [JdkHttpClient.execute0] - Executing request: (POST) /v1.41/images/create
19:22:13.931 DEBUG [HttpClientImpl$SelectorManager.run] - [HttpClient-1-SelectorManager] [1s 205ms] HttpClientImpl(1) next timeout: 0
19:22:13.932 DEBUG [HttpClientImpl$SelectorManager.run] - [HttpClient-1-SelectorManager] [1s 205ms] HttpClientImpl(1) next expired: 1199068
19:22:13.932 DEBUG [HttpClientImpl$SelectorManager.run] - [HttpClient-1-SelectorManager] [1s 205ms] HttpClientImpl(1) Next deadline is 3000
19:22:13.932 DEBUG [JdkHttpClient.execute0] - Ending request (POST) /v1.41/images/create in 0ms
19:22:13.932 DEBUG [SeleniumSpanExporter$1.lambda$export$4] - SpanData{spanContext=ImmutableSpanContext{traceId=568cd549092fb543c58cf8a78f876458, spanId=5c39ea56a5cc69ec, traceFlags=01, traceState=ArrayBasedTraceState{entries=[]}, remote=false, valid=true}, parentSpanContext=ImmutableSpanContext{traceId=00000000000000000000000000000000, spanId=0000000000000000, traceFlags=00, traceState=ArrayBasedTraceState{entries=[]}, remote=false, valid=false}, resource=Resource{schemaUrl=null, attributes={service.name="unknown_service:java", telemetry.sdk.language="java", telemetry.sdk.name="opentelemetry", telemetry.sdk.version="1.36.0"}}, instrumentationScopeInfo=InstrumentationScopeInfo{name=default, version=null, schemaUrl=null, attributes={}}, name=httpclient.execute, kind=INTERNAL, startEpochNanos=1711740133932100937, endEpochNanos=1711740133932641559, attributes=AttributesMap{data={span.kind=client, http.target=/v1.41/images/create, http.method=POST}, capacity=128, totalAddedValues=3}, totalAttributeCount=3, events=[], totalRecordedEvents=0, links=[], totalRecordedLinks=0, status=ImmutableStatusData{statusCode=UNSET, description=}, hasEnded=true}
19:22:13.933 ERROR [Bootstrap.runMain] - Error during execution
java.lang.reflect.InvocationTargetException
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
	at java.base/java.lang.reflect.Method.invoke(Unknown Source)
	at org.openqa.selenium.grid.Bootstrap.runMain(Bootstrap.java:77)
	at org.openqa.selenium.grid.Bootstrap.main(Bootstrap.java:70)
Caused by: org.openqa.selenium.grid.config.ConfigException: java.lang.reflect.InvocationTargetException
	at org.openqa.selenium.grid.config.MemoizedConfig.getClass(MemoizedConfig.java:119)
	at org.openqa.selenium.grid.node.config.NodeOptions.getNode(NodeOptions.java:181)
	at org.openqa.selenium.grid.node.httpd.NodeServer.createHandlers(NodeServer.java:126)
	at org.openqa.selenium.grid.node.httpd.NodeServer.asServer(NodeServer.java:185)
	at org.openqa.selenium.grid.node.httpd.NodeServer.execute(NodeServer.java:247)
	at org.openqa.selenium.grid.TemplateGridCommand.lambda$configure$4(TemplateGridCommand.java:122)
	at org.openqa.selenium.grid.Main.launch(Main.java:83)
	at org.openqa.selenium.grid.Main.go(Main.java:56)
	at org.openqa.selenium.grid.Main.main(Main.java:41)
	... 6 more
Caused by: java.lang.reflect.InvocationTargetException
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
	at java.base/java.lang.reflect.Method.invoke(Unknown Source)
	at org.openqa.selenium.grid.config.ClassCreation.callCreateMethod(ClassCreation.java:51)
	at org.openqa.selenium.grid.config.MemoizedConfig.lambda$getClass$4(MemoizedConfig.java:104)
	at java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(Unknown Source)
	at org.openqa.selenium.grid.config.MemoizedConfig.getClass(MemoizedConfig.java:99)
	... 14 more
Caused by: java.lang.IllegalArgumentException: non-positive contentLength: 0
	at java.net.http/java.net.http.HttpRequest$BodyPublishers.fromPublisher(Unknown Source)
	at org.openqa.selenium.remote.http.jdk.JdkHttpMessages.notChunkingBodyPublisher(JdkHttpMessages.java:124)
	at org.openqa.selenium.remote.http.jdk.JdkHttpMessages.createRequest(JdkHttpMessages.java:82)
	at org.openqa.selenium.remote.http.jdk.JdkHttpClient.execute0(JdkHttpClient.java:371)
	at org.openqa.selenium.remote.http.AddSeleniumUserAgent.lambda$apply$0(AddSeleniumUserAgent.java:42)
	at org.openqa.selenium.remote.http.Filter.lambda$andFinally$1(Filter.java:55)
	at org.openqa.selenium.remote.http.jdk.JdkHttpClient.execute(JdkHttpClient.java:352)
	at org.openqa.selenium.remote.tracing.TracedHttpClient.execute(TracedHttpClient.java:54)
	at org.openqa.selenium.docker.v1_41.PullImage.apply(PullImage.java:62)
	at org.openqa.selenium.docker.v1_41.V141Docker.getImage(V141Docker.java:79)
	at org.openqa.selenium.docker.Docker.lambda$getImage$0(Docker.java:50)
	at java.base/java.util.Optional.map(Unknown Source)
	at org.openqa.selenium.docker.Docker.getImage(Docker.java:50)
	at org.openqa.selenium.grid.node.docker.DockerOptions.lambda$loadImages$5(DockerOptions.java:259)
	at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(Unknown Source)
	at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.exec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool.scan(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
Exception in thread "Thread-0" java.lang.NullPointerException: Cannot invoke "org.openqa.selenium.grid.node.Node.getStatus()" because "this.node" is null
	at org.openqa.selenium.grid.node.httpd.NodeServer.lambda$new$0(NodeServer.java:78)
	at java.base/java.lang.Thread.run(Unknown Source)
2024-03-29 19:22:14,277 INFO exited: selenium-grid-docker (exit status 1; not expected)

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

  • Fixed a NullPointerException issue in RemoteNode.java by adding a null check for status.getNodeId() before comparing it with the current node ID. This ensures the node status check does not fail when status.getNodeId() is null.

Changes walkthrough

Relevant files
Bug fix
RemoteNode.java
Prevent NullPointerException in RemoteNode Status Check   

java/src/org/openqa/selenium/grid/node/remote/RemoteNode.java

  • Added a null check for status.getNodeId() before comparing node IDs to
    prevent NullPointerException.
  • +1/-1     

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

    Fixed: SeleniumHQ/docker-selenium#2175
    Exception in thread "Thread-0" java.lang.NullPointerException: Cannot invoke "org.openqa.selenium.grid.node.Node.getStatus()" because "this.node" is null
    
    Signed-off-by: Viet Nguyen Duc <[email protected]>
    Copy link
    Contributor

    PR Description updated to latest commit (5fa0497)

    Copy link
    Contributor

    PR Review

    ⏱️ Estimated effort to review [1-5]

    2, because the change is small and straightforward, focusing on a specific condition check within the check() method of the RemoteNode.java file. The logic is not complex, but understanding the context of the change requires familiarity with the Selenium Grid architecture and the issue it addresses.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Regression: While the added null check before comparing node IDs aims to prevent a NullPointerException and ensure the node ID is present, it's important to verify that this change doesn't inadvertently allow nodes with null IDs to be considered valid in scenarios where they shouldn't be. This could potentially introduce a regression if other parts of the system rely on the assumption that a node's ID will never be null at this point in the code.

    🔒 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=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

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

    Copy link
    Contributor

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Improve readability by using Objects.nonNull for null-checking.

    Consider using Objects.nonNull(status.getNodeId()) for null-checking instead of
    status.getNodeId() != null. It's more readable and aligns with the use of Objects.equals
    for equality checks.

    java/src/org/openqa/selenium/grid/node/remote/RemoteNode.java [282]

    -if (status.getNodeId() != null && !Objects.equals(getId(), status.getNodeId())) {
    +if (Objects.nonNull(status.getNodeId()) && !Objects.equals(getId(), status.getNodeId())) {
     

    ✨ 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=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

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

    @VietND96
    Copy link
    Member Author

    VietND96 commented Apr 8, 2024

    image

    I also have a quick test locally to verify the fix

    Copy link
    Member

    @diemol diemol left a comment

    Choose a reason for hiding this comment

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

    @VietND96, thanks for checking this!

    Your change probably fixes the the outer exception, but I see this one:

    Caused by: java.lang.IllegalArgumentException: non-positive contentLength: 0

    I think the issue is triggered by the removal of this code. What do you think?

    @VietND96
    Copy link
    Member Author

    VietND96 commented Apr 9, 2024

    Oh ok, let me double-check again

    Copy link

    @A1exKH A1exKH left a comment

    Choose a reason for hiding this comment

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

    LGTM.

    Fixed exception: Caused by: java.lang.IllegalArgumentException: non-positive contentLength: 0
    
    Signed-off-by: Viet Nguyen Duc <[email protected]>
    @VietND96
    Copy link
    Member Author

    @diemol, you're right! I just added another check for case length = 0. However a new exception raised

    03:16:58.799 DEBUG [SocketTube$InternalWriteSubscriber$WriteSubscription.requestMore] - [JdkHttpClient-0-3] [2s 314ms] SocketTube(3) write: requesting more...
    03:16:58.799 DEBUG [SocketTube.debugState] - [JdkHttpClient-0-3] [2s 314ms] SocketTube(3) leaving requestMore:  Reading: [ops=1, demand=1, stopped=false], Writing: [ops=0, demand=1]
    03:16:58.799 DEBUG [Http1Response.lambda$readBody$2] - [JdkHttpClient-0-4] [2s 314ms] Http1Response(id=9, PlainHttpConnection(SocketTube(3))) Finished reading body: READING_BODY
    03:16:58.799 DEBUG [Http1Response$ClientRefCountTracker.tryRelease] - [JdkHttpClient-0-4] [2s 315ms] Http1Response(id=9, PlainHttpConnection(SocketTube(3))) Operation finished: decrementing ref count for jdk.internal.net.http.HttpClientImpl@acc583f(1)
    03:16:58.799 DEBUG [Http1AsyncReceiver.flush] - [JdkHttpClient-0-4] [2s 315ms] Http1AsyncReceiver(SocketTube(3)) Delegate done: 0
    03:16:58.799 DEBUG [Http1AsyncReceiver.flush] - [JdkHttpClient-0-4] [2s 315ms] Http1AsyncReceiver(SocketTube(3)) Got 0 bytes for delegate null
    03:16:58.802 DEBUG [JdkHttpClient.execute0] - Ending request (POST) /v1.41/images/create in 955ms
    03:16:58.804 DEBUG [SeleniumSpanExporter$1.lambda$export$4] - SpanData{spanContext=ImmutableSpanContext{traceId=42c110f68b0f6e860fa[2109](https://github.com/NDViet/docker-selenium/actions/runs/8625419739/job/23641993212#step:11:2110)4b7b20804, spanId=c68458144c455fce, traceFlags=01, traceState=ArrayBasedTraceState{entries=[]}, remote=false, valid=true}, parentSpanContext=ImmutableSpanContext{traceId=00000000000000000000000000000000, spanId=0000000000000000, traceFlags=00, traceState=ArrayBasedTraceState{entries=[]}, remote=false, valid=false}, resource=Resource{schemaUrl=null, attributes={service.name="unknown_service:java", telemetry.sdk.language="java", telemetry.sdk.name="opentelemetry", telemetry.sdk.version="1.36.0"}}, instrumentationScopeInfo=InstrumentationScopeInfo{name=default, version=null, schemaUrl=null, attributes={}}, name=httpclient.execute, kind=INTERNAL, startEpochNanos=1712719017847168436, endEpochNanos=1712719018804416290, attributes=AttributesMap{data={span.kind=client, http.target=/v1.41/images/create, http.status_code=404, http.method=POST}, capacity=128, totalAddedValues=4}, totalAttributeCount=4, events=[], totalRecordedEvents=0, links=[], totalRecordedLinks=0, status=ImmutableStatusData{statusCode=ERROR, description=Kind: NOT_FOUND Description:}, hasEnded=true}
    03:16:58.804 INFO [PullImage.apply] - Have response from server
    03:16:58.805 ERROR [Bootstrap.runMain] - Error during execution
    java.lang.reflect.InvocationTargetException
    	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
    	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
    	at java.base/java.lang.reflect.Method.invoke(Unknown Source)
    	at org.openqa.selenium.grid.Bootstrap.runMain(Bootstrap.java:77)
    	at org.openqa.selenium.grid.Bootstrap.main(Bootstrap.java:70)
    Caused by: org.openqa.selenium.grid.config.ConfigException: java.lang.reflect.InvocationTargetException
    	at org.openqa.selenium.grid.config.MemoizedConfig.getClass(MemoizedConfig.java:119)
    	at org.openqa.selenium.grid.node.config.NodeOptions.getNode(NodeOptions.java:181)
    	at org.openqa.selenium.grid.node.httpd.NodeServer.createHandlers(NodeServer.java:126)
    	at org.openqa.selenium.grid.node.httpd.NodeServer.asServer(NodeServer.java:185)
    	at org.openqa.selenium.grid.node.httpd.NodeServer.execute(NodeServer.java:247)
    	at org.openqa.selenium.grid.TemplateGridCommand.lambda$configure$4(TemplateGridCommand.java:122)
    	at org.openqa.selenium.grid.Main.launch(Main.java:83)
    	at org.openqa.selenium.grid.Main.go(Main.java:56)
    	at org.openqa.selenium.grid.Main.main(Main.java:41)
    	... 6 more
    Caused by: java.lang.reflect.InvocationTargetException
    	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
    	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
    	at java.base/java.lang.reflect.Method.invoke(Unknown Source)
    	at org.openqa.selenium.grid.config.ClassCreation.callCreateMethod(ClassCreation.java:51)
    	at org.openqa.selenium.grid.config.MemoizedConfig.lambda$getClass$4(MemoizedConfig.java:104)
    	at java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(Unknown Source)
    	at org.openqa.selenium.grid.config.MemoizedConfig.getClass(MemoizedConfig.java:99)
    	... 14 more
    Caused by: org.openqa.selenium.docker.DockerException: manifest for selenium/standalone-chrome:trunk-20240410 not found: manifest unknown: manifest unknown
    	at org.openqa.selenium.docker.v1_41.PullImage.apply(PullImage.java:76)
    	at org.openqa.selenium.docker.v1_41.V141Docker.getImage(V141Docker.java:79)
    	at org.openqa.selenium.docker.Docker.lambda$getImage$0(Docker.java:50)
    	at java.base/java.util.Optional.map(Unknown Source)
    	at org.openqa.selenium.docker.Docker.getImage(Docker.java:50)
    	at org.openqa.selenium.grid.node.docker.DockerOptions.lambda$loadImages$5(DockerOptions.java:259)
    	at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(Unknown Source)
    	at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.exec(Unknown Source)
    	at java.base/java.util.concurrent.ForkJoinTask.doExec(Unknown Source)
    	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(Unknown Source)
    	at java.base/java.util.concurrent.ForkJoinPool.scan(Unknown Source)
    	at java.base/java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
    	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
    Exception in thread "Thread-0" java.lang.NullPointerException: Cannot invoke "org.openqa.selenium.grid.node.Node.getStatus()" because "this.node" is null
    	at org.openqa.selenium.grid.node.httpd.NodeServer.lambda$new$0(NodeServer.java:78)
    	at java.base/java.lang.Thread.run(Unknown Source)

    @diemol
    Copy link
    Member

    diemol commented Apr 10, 2024

    Does the error come from trying to pull an image that does not exist?

    Caused by: org.openqa.selenium.docker.DockerException: manifest for selenium/standalone-chrome:trunk-20240410 not found: manifest unknown: manifest unknown

    @VietND96
    Copy link
    Member Author

    @diemol, there is something wrong with the test configs on CI. I just updated it, now tests passed - https://github.com/NDViet/docker-selenium/actions/runs/8632690937/job/23663997230
    You can review and merge this PR

    @baflQA
    Copy link
    Contributor

    baflQA commented Apr 10, 2024

    @diemol can we count on a hotfix to 4.19 release? Or do we have to wait for 4.20 for this fix?

    @diemol
    Copy link
    Member

    diemol commented Apr 10, 2024

    4.20 is coming next week.

    Copy link
    Member

    @diemol diemol left a comment

    Choose a reason for hiding this comment

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

    Thank you, @VietND96!

    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]: Tests failing since docker grid update to version 4.19
    4 participants