[
https://issues.apache.org/jira/browse/TINKERPOP-2708?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17547028#comment-17547028
]
ASF GitHub Bot commented on TINKERPOP-2708:
-------------------------------------------
xiazcy opened a new pull request, #1680:
URL: https://github.com/apache/tinkerpop/pull/1680
As suggested in https://issues.apache.org/jira/browse/TINKERPOP-2708,
deprecated and removed the functionality of the connection option
`connectOnStartup`. Since it is generally not good to return promises in
constructors, removing the call to `open()` in the `Connection` constructor
seemed to be the way to resolve unhandledRejection and any potential race
conditions.
There is a slight API behaviour change as a result. Setting
`connectOnStartup` true now will log a deprecated warning to the user. The
`Connection` constructor will no longer initiate connection in the background
by default, so users will need to call `open()` explicitly after creating a
connection object. However, open() is called anyways when connection submits
traversals, removing this shouldn't affect any normal traversal functionality.
As this may be considered a breaking change, I added a change-log entry, are
there additional docs I should edit?
As a side note, this is related to
https://issues.apache.org/jira/browse/TINKERPOP-2381. I believe removing
connectOnStartup functionality resolves the issues listed. I'm not sure if
there is a need to refactor how Client invokes `connection.open()`, since
inside `connection.open()`, it does check if it has been called previously.
> 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)
