[
https://issues.apache.org/jira/browse/TINKERPOP-2708?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17550945#comment-17550945
]
ASF GitHub Bot commented on TINKERPOP-2708:
-------------------------------------------
spmallette commented on PR #1680:
URL: https://github.com/apache/tinkerpop/pull/1680#issuecomment-1148538843
> As this may be considered a breaking change, I added a change-log entry,
are there additional docs I should edit?
breaking changes are typically documented in the Upgrade Documentation so
that we can call attention to important changes more easily with greater detail
than the CHANGELOG - it is probably worth adding an item to say that this
parameter no longer does anything and that you would want to call `open()`
explicitly to "connect on startup". also, if it is a breaking change, there
should be a "breaking" label on the JIRA. finally, i would assume the
deprecation to be removed on merge to master for 3.7.0, so perhaps you should
consider a follow-on PR to get rid of the warning message and parameter all
together on that branch.
separately, there is a merge commit in the history. could you please remove
that? other than that VOTE +1
> 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)
