You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by "gion (GitHub)" <gi...@apache.org> on 2019/09/30 11:32:51 UTC

[GitHub] [tinkerpop] gion opened pull request #1202: TINKERPOP-2290 Add a way to catch a connection error

and prevent the program from exiting whenever the db disconnects

[ Full content available at: https://github.com/apache/tinkerpop/pull/1202 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] jorgebay commented on issue #1202: TINKERPOP-2290 Add a way to catch a connection error

Posted by "jorgebay (GitHub)" <gi...@apache.org>.
@gion Sorry I didn't reply to you before, I agree that is useful to expose the `Connection` events.

[ Full content available at: https://github.com/apache/tinkerpop/pull/1202 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] spmallette commented on issue #1202: TINKERPOP-2290 Add a way to catch a connection error

Posted by "spmallette (GitHub)" <gi...@apache.org>.
No specific feedback on the technical nature of the change itself but administratively speaking - The Travis failures are due to a problem on tp34  and master branches, that came in after a merge of a PR on tp33 what wasn't compatible with those downstream branches. That should be fixed now. This one will probably need to be rebased for it to pass through the Travis build. When rebased, I assume that this PR should target the tp33 branch rather than master so that changes can go to 3.3.x and 3.4.x (`master` is for 3.5.0).

[ Full content available at: https://github.com/apache/tinkerpop/pull/1202 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] gion commented on issue #1202: TINKERPOP-2290 Add a way to catch a connection error

Posted by "gion (GitHub)" <gi...@apache.org>.
In my use case, the node process was exiting whenever the db had a connection issue.
And, of course, the underlying issue was the fact that the connection was emitting an uncatched `error` event.
My first approach was to handle that `error` event, so I exposed a way to intercept it, but the cleaner solution is, as @jorgebay suggested, to update the API to rename the error event to something that doesn't mess with the node process. 
I did leave the initial commit here, as I find it useful. Here's how I would handle a reconnect with this new change:
```javascript
const gremlin = require('gremlin');
const DriverRemoteConnection = gremlin.driver.DriverRemoteConnection;

const MAX_RETRY_COUNT = 3;
const RETRY_DELAY = 1000;

let retryCount = 0;
let remoteConnection;

// this is the initial function that defines the driver (with it's own event emitters)
// and attaches error event listeners before opening a connection
async function setup() {
  remoteConnection = new DriverRemoteConnection('ws://localhost:8182/gremlin');
  remoteConnection.on('socketError', handleDisconnect)
  await connectAndRun()
}

// the main error handler
// it will trigger when the db is disconnected
function handleDisconnect() {
  // retry to connect and do the logic
  setTimeout(connectAndRun, RETRY_DELAY)
}

async function connectAndRun() {
  retryCount++
  console.log(`Connecting... try #${retryCount}`)

  // exit the process if we retried to connect too many times
  if (retryCount > MAX_RETRY_COUNT) {
    throw new Error(`retried ${MAX_RETRY_COUNT} times... let's quit`)
  }

  try {
    // open the connection
    await remoteConnection.open();

    // task block
    // do whatever needs to be done
    // ...
    await remoteConnection.close();
  } catch (err) {
    console.error('something bad has happened 😕');
  }
}
```

Please let me know if you think I should split this PR into 2 separate ones.


[ Full content available at: https://github.com/apache/tinkerpop/pull/1202 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] jorgebay commented on issue #1202: TINKERPOP-2290 Add a way to catch a connection error

Posted by "jorgebay (GitHub)" <gi...@apache.org>.
Thanks @gion for looking into this!

It's a good idea to expose a way to subscribe to connection events.

In the case of errors, I think it would be best to avoid [emitting `'error'` events](https://github.com/apache/tinkerpop/blob/3.4.3/gremlin-javascript/src/main/javascript/gremlin-javascript/lib/driver/connection.js#L216) and instead use `'socketError'`, that way we don't default to an uncaught error when the user is not listening to the `'error'` event, wdyt?

[ Full content available at: https://github.com/apache/tinkerpop/pull/1202 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] gion commented on issue #1202: TINKERPOP-2290 Add a way to catch a connection error

Posted by "gion (GitHub)" <gi...@apache.org>.
It appears that [the docker build has failed in travis](https://travis-ci.org/apache/tinkerpop/jobs/592491432), but it might be related to a timeout. Could anyone re-trigger the build, so I don't have to push empty commits to do it?

[ Full content available at: https://github.com/apache/tinkerpop/pull/1202 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] asfgit closed pull request #1202: TINKERPOP-2290 Add a way to catch a connection error

Posted by "asfgit (GitHub)" <gi...@apache.org>.
[ pull request closed by asfgit ]

[ Full content available at: https://github.com/apache/tinkerpop/pull/1202 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org

[GitHub] [tinkerpop] gion commented on issue #1202: TINKERPOP-2290 Add a way to catch a connection error

Posted by "gion (GitHub)" <gi...@apache.org>.
In my use case, the node process was exiting whenever the db had a connection issue.
And, of course, the underlying issue was the fact that the connection was emitting an uncatched `error` event.
My first approach was to handle that `error` event, so I exposed a way to intercept it, but the cleaner solution is, as @jorgebay suggested, to update the API to rename the error event to something that doesn't mess with the node process. 
I did leave the initial commit here, as I find it useful. Here's how I would handle a reconnect with this new change:
```javascript
const gremlin = require('gremlin');
const DriverRemoteConnection = gremlin.driver.DriverRemoteConnection;

const MAX_RETRY_COUNT = 3;
const RETRY_DELAY = 1000;

let retryCount = 0;
let remoteConnection;

// this is the initial function that defines the driver (with it's own event emitters)
// and attaches error event listeners before opening a connection
async function setup() {
  remoteConnection = new DriverRemoteConnection('ws://localhost:8182/gremlin');
  remoteConnection.on('socketError', handleDisconnect)
  await connectAndRun()
}

// the main error handler
// it will trigger when the db is disconnected
async function handleDisconnect() {
  // retry to connect and do the logic
  setTimeout(connectAndRun, RETRY_DELAY)
}

async function connectAndRun() {
  retryCount++
  console.log(`Connecting... try #${retryCount}`)

  // exit the process if we retried to connect too many times
  if (retryCount > MAX_RETRY_COUNT) {
    throw new Error(`retried ${MAX_RETRY_COUNT} times... let's quit`)
  }

  try {
    // open the connection
    await remoteConnection.open();

    // task block
    // do whatever needs to be done
    // ...
    await remoteConnection.close();
  } catch (err) {
    console.error('something bad has happened 😕');
  }
}
```

Please let me know if you think I should split this PR into 2 separate ones.


[ Full content available at: https://github.com/apache/tinkerpop/pull/1202 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org