Support request timeout as option

[phamann]

Whilst trying to tune our DB connection, we have realised it would be useful to expose Request's max timeout option in the options hash supplied when creating a new GraphDatabase instance.

Such as:

const db = new neo4j.GraphDatabase({
    url: 'http://foo.bar.com:7474/',
    timeout: 5000 
})

Like the new headers support in v2, this could also then be overridden at a request level.

I'm happy to attempt submitting this change as a PR, but wanted to post here first and discuss the desired interface before doing the work.

In the interim we are suppling a custom newhttp.Agent` with keep-alive and max sockets enabled. Any other suggestions you have for http tuning would be great.

[aseemk]

Hey @phamann,

Thanks for the suggestion — it's a good one. I have a few thoughts.

One is that I always go back and forth on whether to expose Request's raw options directly. There are a lot of good ones, so it'd be powerful for cases where people need custom/uncommon functionality. But ultimately, I lean towards not exposing that implementation detail directly, in case it changes at some point. (E.g. with v2, I took a stab at not using Request at all (but ultimately brought it back for easy gzip support). And in the future, I might consider Superagent, for better browser-side compat.)

With request timeout specifically, that's definitely important, but my understanding and impression is that that's better to specify at the Neo4j config level (which is what we at @fiftythree do too):

http://www.markhneedham.com/blog/2013/10/17/neo4j-setting-query-timeout/

The two relevant configs being conf/neo4j.properties > execution_guard_enabled and conf/neo4j-server.properties > org.neo4j.server.webserver.limit.executiontime.

I believe those are better because it ensures that Neo4j actually stops processing the query, as opposed to just aborting the connection (which is what Request's timeout would do). (To be fair, aborting the connection might "just work" too, but I'm not sure.)

The blog post also mentions a Max-Execution-Time header, and it appears that does work still, with the new REST endpoint that node-neo4j v2 uses. So you should be able to use that for individual queries already! 😄

We've actually had issue #13 open for a long time, to support passing this header. Good to know this is fixed in v2.

Hope all this helps! Let me know if you have any other q's.

[aseemk]

I'll go ahead close this question as answered for now, but feel free to re-open.

[phamann]

@aseemk Thank you very much for the detailed response, it's greatly appreciated.

We didn't know about the Max-Execution-Time header and thus will experiment with that. And had already discuss setting a more strict query time within Neo4j directly internally within the team so good to hear you also do that.

Therefore, happy for you to close.