Skip to content

driver.onCompleted not executing; driver.onError not executing too. #234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Jabher opened this issue Apr 23, 2017 · 13 comments
Closed

driver.onCompleted not executing; driver.onError not executing too. #234

Jabher opened this issue Apr 23, 2017 · 13 comments

Comments

@Jabher
Copy link

Jabher commented Apr 23, 2017

So, I've started using this driver after a few months of not using, and everything was crashing - it looked like driver is not instantating.
After some investigations I've discovered that my app was waiting for driver.onCompleted to execute, but it never executed.

However, if I would remove line of waiting driver.onCompleted to run, everything starts to work fine.
Same for driver.onError. I cannot detect whether credentials are bad or I can perform the requests properly.
IDK why this is happening as I've installed older (1.1.0 version) which was working fine with this API. Also I've used neo4j 3.0.x (cannot definitely recall the exact version) and then upgraded to 3.1.4, thinking that it can be newer driver-older db issue, still did not help.

▶ node    
> const {v1: neo4j} = require('neo4j-driver')
undefined
> driver = neo4j.driver("bolt://localhost", neo4j.auth.basic("neo4j", "password1")); driver.onError = console.log.bind(console, 'err');
[Function: bound bound log]
> driver = neo4j.driver("bolt://localhost", neo4j.auth.basic("neo4j", "password")); driver.onCompleted = console.log.bind(console, 'completed');
[Function: bound bound log]
> 
@lutovich
Copy link
Contributor

Hello @Jabher,

Thanks for reporting this problem. I can reproduce the described behaviour.
Driver 1.2 tries to acquire connections as lazily as possible. This means driver.session() will not try to establish connection as it did before before. Only session#run(), session#readTransaction(), session#writeTransaction() and session#beginTransaction() will actually try to connect. Successful connection results in driver.onCompleted invocation and connection error results in driver.onError invocation.

When driver is used, callbacks seem to be invoked properly:

> const {v1: neo4j} = require('neo4j-driver')
undefined
> const driver = neo4j.driver("bolt://localhost", neo4j.auth.basic("neo4j", "test")); driver.onCompleted = console.log.bind(console, 'completed');
[Function: bound bound ]
> const session = driver.session()
undefined
> session.run('return 1')
Result {
 ...
}
> completed { server: 'Neo4j/3.1.3' }

Maybe you could use smth similar as a workaround?
We will investigate and fix this problem.

@Jabher
Copy link
Author

Jabher commented Apr 23, 2017

Hmm, this sounds like a sound workaround. Maybe it's even better - it's also checking that requests are performing as expected. However, I'd probably recommend to at least re-think the names of callback hooks :)
Thanks!

@Jabher
Copy link
Author

Jabher commented Apr 23, 2017

BTW, awesome update, I have some performance benchmark in my lib - looks like querying boosted up 3x times - from 250-300 query requests (synthetic env) per second up to 700-900.

@lutovich
Copy link
Contributor

Maybe you can use code similar to this:

const {v1: neo4j} = require('neo4j-driver');
const driver = neo4j.driver(...);
const session = driver.session();
session.run('RETURN 1').then(() => {
  // proceed with application startup
}).catch(error => {
  // fail application startup
})

if request/query verification at startup is desired? Then driver.onCompleted and driver.onError can be omitted.

It is great to hear about the performance improvement :)
Did you only upgrade driver or database as well?

@Jabher
Copy link
Author

Jabher commented Apr 23, 2017

Already did that: Jabher/agregate@6780a44

About performance - I've updated both driver and db (3.0.x -> 3.1.4), so I'm not sure about whom to blame, but still it's working good.

@Jabher
Copy link
Author

Jabher commented Apr 23, 2017

request/query verification is desired, as long as I really, really, really prefer to raise an wrong credentials exception during the connection - before server hot-swap - instead of catching an error on first user request :)

@simonegabbriellini
Copy link

Hi @lutovich ,

I am facing the same problem, can you please help me to understand how to solve this in a browser context? What I am doing right now follows what I have found on the dev docs:
neo4j = neo4j.v1;
var driver = neo4j.driver("bolt://localhost:7474", neo4j.auth.basic("neo4j", "neo4j"));
driver.onCompleted = function () {
console.log("I am here"); // this never gets printed in the console
};

I am a Neo4J newbie, so thanks very much for your help!

@lutovich
Copy link
Contributor

lutovich commented Jun 21, 2017

Hi @simonegabbriellini,

You could simply skip the driver.onCompleted part. Try:

var driver = neo4j.driver("bolt://localhost:7474", neo4j.auth.basic("neo4j", "neo4j"));
// use driver to create sessions here

@simonegabbriellini
Copy link

@lutovich I get your suggestion, which simply implies to just ignore the usage examples code... but what I don't get is what am I missing? I am doing everything according to the example usage from here: https://github.com/neo4j/neo4j-javascript-driver
Is this a bug or, I repeat myself here, am I missing something?

@lutovich
Copy link
Contributor

@simonegabbriellini I think it's a bug. Driver does not verify connectivity on creation and driver.onCompleted does not get called. It is not strictly required to use this callback. You could as well use a dummy query, like suggested earlier.

@shanelacey
Copy link

Hi @lutovich , do you know if this was fixed in 1.5.0-alpha01?

@lutovich
Copy link
Contributor

Hi @shanelacey,

I do not think this problem has been fixed. Could you please use RETURN 1 workaround for now? Hope we'll fix it by 1.5 GA.

@shanelacey
Copy link

Hey @lutovich,

Thanks for getting back to me! Thats ok I just wanted to check. And yeah I'm using that for now. Looking forward to it, and thanks again for the update.

ponyatov added a commit to ponyatov/article that referenced this issue Sep 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants