Skip to content
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

Invoke pending callbacks in case of an error #60

Merged
merged 1 commit into from
Oct 14, 2016
Merged

Invoke pending callbacks in case of an error #60

merged 1 commit into from
Oct 14, 2016

Conversation

petar-iv
Copy link
Contributor

Fixes issue #53

Currently the following code:

var hdb = require('hdb');

var client = hdb.createClient({ /* options here */ });

client.on('error', function (err) {
  console.log('Error event:', err.message);
});

client.on('close', function (hadError) {
  console.log('Close event, had error:', hadError);
});

client.connect(function (err) {
  if (err) {
    return console.log('Connect Error:', err.message);
  }
  console.log('Successful connection');
  console.log('Closing client...');
  client.close();

  client.exec('SELECT * FROM DUMMY', function cb1(err, rs) {
    console.log('-- cb1', err && err.message);
  });

  client.exec('SELECT * FROM DUMMY', function cb2(err, rs) {
    console.log('-- cb2', err && err.message);
  });
});

Results in:

Successful connection
Closing client...
Error event: This socket is closed
Close event, had error: false

Thus, the callbacks of the 2 client.exec invocations are never called. The same thing (not calling the callback) happens in issue #53 as well.

With this fix the output of the same code will be:

Successful connection
Closing client...
-- cb1 This socket is closed
-- cb2 This socket is closed
Close event, had error: false

Thus, the pending callbacks (of the 2 client.exec invocations) are invoked with the corresponding error.

Regarding issue #53, the output of the code (with XSEngine port provided instead of DB port) will be:

Connect Error: Connection closed by server
Close event, had error: false

@holgerkoser
Copy link
Member

Does this PR handles the case that there are pending callbacks in the queue?

@petar-iv
Copy link
Contributor Author

I think yes.

Let's suppose that the socket has been closed and we have the following code:

client.exec('SELECT * FROM DUMMY', function cb1() { ... });
client.exec('SELECT * FROM DUMMY', function cb2() { ... });

Both of these will be added to the queue. In this particular case the callback of the task is the handleReply function. When we try to write on the socket, an error is emitted. connection._state.receive contains the callback of the task (in our case a function that wraps handleReply) and then this callback is being called. Since the queue is still running, the second task will also run and try to write on the socket which leads to one more error being emitted.

Let me know in case there is something more to that change.
Thanks a lot.

@holgerkoser
Copy link
Member

You are right. I think this is a good improvement. Thank you two.

@alexpenev-s alexpenev-s merged commit bf67737 into SAP:master Oct 14, 2016
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

Successfully merging this pull request may close these issues.

None yet

3 participants