[jira] [Commented] (TINKERPOP-2708) unhandledRejection upon connection failure

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

    [ 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)