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

Don't prevent NodeJS from closing to run acquisition timeout error #1196

Merged
merged 1 commit into from
May 31, 2024

Conversation

CarsonF
Copy link
Contributor

@CarsonF CarsonF commented May 29, 2024

Currently my node process will wait 60 seconds (I know it's configurable) to close because this timeout is waiting to fire a timeout error. This stops referencing counting this callback, so it won't prevent the process from closing. This will still process if there are other executions in the event loop, so all this affects if node is waiting on a timeout that will never be needed.


Digging more into this the wait time happens because the socket never emits the logon success.
60% of the time I see

C: LOGON { ... }

But no bytes are ever read / the data event is never fired.
This just "hangs" waiting for logon response, until the timeout is fired. I'm not sure why this happens. I haven't indicated to node in any way of my intention to close the process. Yet somehow I suspect it is aware of this.

The other 40% the logon / connection init process works fine:

C: LOGON { ... }
NodeChannel._conn.data <Buffer 00 56 b1 70 a3 86 73 65 72 76 65 72 8c 4e 65 6f 34 6a 2f 35 2e 32 30 2e 30 8d 63 6f 6e 6e 65 63 74 69 6f 6e 5f 69 64 87 62 6f 6c 74 2d 32 37 85 68 69 ... 47 more bytes>
S: SUCCESS {"signature":112,"fields":[{"server":"Neo4j/5.20.0","connection_id":"bolt-27","hints":{"connection.recv_timeout_seconds":{"low":120,"high":0}}}]}

In these cases the process cleans up rapidly.

@bigmontz bigmontz self-requested a review May 30, 2024 09:56
@bigmontz
Copy link
Contributor

Hej @CarsonF, there is some errors in the pipelines which I will need to investigate.

Could you share the Node version you used?

@CarsonF
Copy link
Contributor Author

CarsonF commented May 30, 2024

v22.0.0

Copy link
Contributor

@bigmontz bigmontz left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, @CarsonF .

However, there is a problem with the code when running in browser, see my other comment in the PR.

Apart of this, you need to run npm run build::deno for sync the deno version of the driver.

packages/bolt-connection/src/pool/pool.js Outdated Show resolved Hide resolved
@bigmontz bigmontz merged commit 75d3040 into neo4j:5.0 May 31, 2024
37 checks passed
@bigmontz
Copy link
Contributor

Thanks for your contribution, @CarsonF. The changes will be released in the version 5.22.0 of the driver.

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.

2 participants