Skip to content

Conversation

@efkan
Copy link
Contributor

@efkan efkan commented Feb 27, 2017

When providing an Id from an in-memory database, accessing the database over the network and getting the id may cause to skip id assignment.

In this case id will be undefined. Because of this reason engine.io fall into an endless loop.
To overcome this issue async-await combination can be used.

The kind of change this PR does introduce

  • a new feature
  • a bug fix
  • an update to the documentation
  • a code change that improves performance
  • other

Current behaviour

generateId may be skipped as undefined because of id generating operations.

New behaviour

By using await option, handshake method can be waited to complete generateId() function.

Other information (e.g. related issues)

However, async-await combination doesn't work on former Node.js versions.

When providing an Id from an in-memory database, accessing the database over the network and getting the id may cause to skip `id` assignment. 

In this case id will be `undefined`. Because of this reason engine.io fall into an endless loop.
To overcome this issue async-await combination can be used. 

Also async-await solution is not a breaking change.
@darrachequesne
Copy link
Member

How about:

this.generateId(req, function (id) {
  // ...
});

@efkan
Copy link
Contributor Author

efkan commented Feb 28, 2017

@darrachequesne , it didn't come to mind!

On the other hand I would have to wrap the rest of the code into a callback function.
I guess async-await provides more readability. But actually it doesn't matter for me.

Thank you!

@efkan
Copy link
Contributor Author

efkan commented Feb 28, 2017

I couldn't pass the tests on Travis.
So, I'm closing the PR.

@efkan efkan closed this Feb 28, 2017
@efkan
Copy link
Contributor Author

efkan commented Mar 1, 2017

@darrachequesne , I've tried to commit my PR. Unfortunately with a callback it throws different error. They may be reletad with this keyword. Meybe not.

As a result, I gave up for now.

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.

2 participants