[
https://issues.apache.org/jira/browse/TINKERPOP-2708?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17551799#comment-17551799
]
ASF GitHub Bot commented on TINKERPOP-2708:
-------------------------------------------
xiazcy commented on code in PR #1680:
URL: https://github.com/apache/tinkerpop/pull/1680#discussion_r892879796
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/connection.js:
##########
@@ -103,13 +103,15 @@ class Connection extends EventEmitter {
this._pingIntervalDelay = this.options.pingInterval || pingIntervalDelay;
this._pongTimeoutDelay = this.options.pongTimeout || pongTimeoutDelay;
- if (this.options.connectOnStartup !== false) {
- this.open();
+ if (this.options.connectOnStartup) {
Review Comment:
Correct, the console warning will not be printed if the user does not set
`connectOnStartup`. It will only be displayed if `{connectOnStartup: true}` is
set.
> unhandledRejection upon connection failure
> ------------------------------------------
>
> Key: TINKERPOP-2708
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2708
> Project: TinkerPop
> Issue Type: Bug
> Components: javascript
> Affects Versions: 3.5.2
> Reporter: Jon Brede Skaug
> Priority: Major
>
> In the Javascript driver is unable to connect to the graph database for
> whatever reason an unhandledRejection warning occurs.
> I have tested this with `new DriverRemoteConnection`
> This is a silent error and it won't be able to catch the error due to the way
> it is handled.
> I've tracked it down to this line:
> [https://github.com/apache/tinkerpop/blob/c22c0141bb7a00f366f929d0e5d3c6379d1004e0/gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/connection.js#L156]
> h2. *Solution suggestion*
> A fairly quick solution (but possibly breaking change) to this is by not
> opening the database in the constructor [(line reference
> L105)|https://github.com/apache/tinkerpop/blob/c22c0141bb7a00f366f929d0e5d3c6379d1004e0/gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/connection.js#L105]
> but instead forcing the user to run the `DriverRemoteConnection.open()`
> after the constructor has been initialized. `DriverRemoteConnection.open()`
> returns a promise which makes more sense and is a bit more intuitive. The
> current error message gives an error about DNS which is increadibly confusing
> without deepdiving into the Gremlin driver code and navigating through 3
> classes to find the culprit. It's also an error which seems a bit more
> harmless than it actually is.
> It'salso possible to set option.connectOnStartup to "false" by default, this
> however will require the user to be aware of the possible failure upon
> setting it to true. I believe forcing the user to run .open() after
> initializing the class may be more robust.
> By doing it this way the user can instead handle the error raised by
> DriverRemoteConnection.open() by using promise.catch() or an async function
> using await. Promise.catch() is as provided:
> {code:java}
> this.drc.open().catch(err => {
> console.log("Unable to open connection to database", err);
> });{code}
> h2. *{{Temporary work around example}}*
>
> {code:java}
> // Using promises
> const drc = new DriverRemoteConnection(url, {connectOnStartup: false});
> drc.open().catch(err => {
> // Handle error upon open, i.e using retry and backoff logic, notify an
> alarm system or setting a global variable to reject requests.
> });{code}
>
> h2. *The issue with not handling the error properly:*
> Not handling the error properly means that if you pass in an invalid URL or
> the gremlin compatible database is down, it won't be able to handle the
> connection error before a transaction is attempted.
> In the future Node.js unhandledRejection will terminate the Node.js process.
> This can cause critical failure of processes upon boot and may even cause
> DDoS situations where processes may flood the gremlin compatible database
> with connection attempts due to processes failing and being reinstated over
> and over by a process monitor.
>
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
