Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

nextTick instead of setImmediate, and fix sync in async #136

Merged
merged 5 commits into from
Jan 3, 2019
Merged

Conversation

jacobheun
Copy link
Contributor

@jacobheun jacobheun commented Dec 19, 2018

The hmac digest method is made to be async but was calling the callback synchronously. In high throughout scenarios this can cause the call stack to bloat, which in severe instances results in stack overflow issues.

This also fixes linting from the latest aegir to fix ci, and removes non jenkins ci as we're not using travis, circle or appveyor anymore.

Relates to https://github.com/libp2p/js-libp2p-switch/issues/287

@ghost ghost assigned jacobheun Dec 19, 2018
@ghost ghost added the status/in-progress In progress label Dec 19, 2018
@achingbrain
Copy link
Member

achingbrain commented Dec 20, 2018

Did you try using process.nextTick instead of setImmediate?

@mcollina's perf review has pointed to some significant improvements we could make by either eliminating async (seems like that might be a big job here) or using process.nextTick over setImmediate.

@mcollina
Copy link

The major advantage of process.nextTick over setImmediate is that it does not wait for a full event loop, reducing latency significantly.

Note that the actual issue is caused by having synchronous API that are exposed as async. I think a long-term solution is to refactor those into synchronous calls.

@jacobheun
Copy link
Contributor Author

I switched the setImmediate usage to nextTick everywhere, so that we can use it when it's supported. For the digest this should help quite a bit since that is called during the unboxing of the crypto stream.

I have updates to bitswap as well but need to get tests passing there. Bitswap does a lot of network calling and is a big offender of sync in async calls. A lot of the sync code was likely included in the async calls to make control flow easier when it was first built out, but now that we're scaling it's causing problems. I agree that needs to get refactored to just be synchronous where it can be.

For things like crypto, where we have interfaces, we'll need to be careful refactoring when the interface needs to by async. Some of our interface implementations are async where others aren't. So we just need to be careful with the interfaces.

@jacobheun jacobheun changed the title revert setImmediate removal nextTick instead of setImmediate, and fix sync in async Dec 20, 2018
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alanshaw
Copy link
Member

alanshaw commented Jan 2, 2019

Use https://caolan.github.io/async/docs.html#nextTick so it works in the browser?

@jacobheun
Copy link
Contributor Author

jacobheun commented Jan 2, 2019

@alanshaw I pushed the change to async/nextTick. The build process should take care of adding checks for process.nextTick, but I made the switch to ensure it's being supported regardless of the build.

@jacobheun
Copy link
Contributor Author

@daviddias or @dignifiedquire can we get a release of this?

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a box of chocolates for the brave soul that takes on doing a find & replace across all our modules to replace setImmediate to nextTick 🍫

@daviddias daviddias merged commit c54ea20 into master Jan 3, 2019
@daviddias daviddias deleted the fix/async branch January 3, 2019 16:13
@ghost ghost removed the status/in-progress In progress label Jan 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants