Skip to content

use setTimeout instead of setImmediate #39

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
wants to merge 1 commit into from

Conversation

abdulhannanali
Copy link

setImmediate is a non standard feature and the behavior is non stable. In the src/utils.js I have replaced the setImmediate function call with an this alternative

setTimeout(async () => {
 await push()
}, 0)

@codecov-io
Copy link

codecov-io commented Jan 22, 2017

Current coverage is 81.81% (diff: 100%)

Merging #39 into master will not change coverage

@@             master        #39   diff @@
==========================================
  Files             3          3          
  Lines            55         55          
  Methods           4          4          
  Messages          0          0          
  Branches         12         12          
==========================================
  Hits             45         45          
  Misses           10         10          
  Partials          0          0          

Powered by Codecov. Last update 09442c1...e6b06c3

@abdulhannanali
Copy link
Author

I also would like to know if this locks functionality will work when we are using multiple processes. What are the alternatives in case we have multiple processes of one micro function running. Can we instead implement an increment functionality in the DB. Db such as redis provides us with this increment function which is atomic.

@mxstbr
Copy link
Member

mxstbr commented Jan 22, 2017

We've thought about moving the atomicity out to the database adapters, see #14 and related issues. Not sure yet if we'll go down that route, /cc @sean-roberts

@sean-roberts
Copy link
Collaborator

I don't think changing this is necessary. This is for sure standard in node. Maybe not in browsers according to mdn, but it's been in node quite a long time. 🤷‍♂️ As far as the locking convo, let's move that to a discussion ticket

@sean-roberts
Copy link
Collaborator

Want to give your thoughts on #40 ?

@mxstbr
Copy link
Member

mxstbr commented Jan 28, 2017

Closing this since we'll probably move forward with #40!

Thanks for taking the time to submit a PR and kicking off this discussion @abdulhannanali!

@mxstbr mxstbr closed this Jan 28, 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

Successfully merging this pull request may close these issues.

4 participants