Repository navigation
jwt.verify() not truly async. #511
Description
Despite the verify action is in fact synchronize, the callback API does not.
It is specifically this line that released Zalgo.
Line 24 in 5a7fa23
Which can be fixed by wrapping it as such
done = function(err, data) {
setImmediate(callback, err, data);
};
Activity
ziluvatar commentedon Aug 16, 2018
That behavior (with process.nextTick() though) was removed in this PR: #302
I will leave the issue open in case the community has strong opinions about re-adding it or keep it as is. I understand the reasons, however, the pain to the event loop was already done while performing the verification synchronously, even doing that is "lying" to the consumer.
There was another discussion to remove the callback-ish style to that function, we didn't do it and now we support to get the secret/key asynchronously which gives sense to the callback now.
vicary commentedon Aug 18, 2018
Forgive my words if the opinion is too strong, but to the best of my knowledge the consensus is already there for decades.
The nature of an asynchronous API should behave like it means, implementation should be hidden behind the API even if it is a so-called lie.
The API is the contract of behavior between the library and its users, and should be followed the moment it goes public as (Asynchronous) ....
IMO #302 should in fact use the (Synchronous) ... version without callback for the desired performance gain, and such performance differences should be documented instead of breaking the API.
MitMaro commentedon Aug 18, 2018
This could be something to address with the next major release of the library. I agree with @vicary in that it is considered a bad practice to have an asynchronous callback that performs synchronously. It personally caused me some issues when I integrated this library into our auth system at work.
One option would be to switch to async-await/promises for the next major release, which would avoid the problem as promises are always asynchronous by design.
vicary commentedon Aug 19, 2018
@MitMaro thanks.
I don't have much on the callback/promise/async controversy because of the disproportionate gain of productivity, the difference in performance is tiny; I like the inline error catching in async/await though.
But the breaking of the current API should be a hotfix instead of next major, there are potential risks for people using them in production.
mcmar commentedon Jan 4, 2019
Hey, I just want to point out that if you ever re-add code to make this async (which you should do), you should also really switch from
process.nextTicktosetImmediate. You can read about the differences here: https://nodejs.org/en/docs/guides/event-loop-timers-and-nexttick/TL;DR
process.nextTickdoesn't allow the event loop time to handle things like I/O buffering before you run the verification function. Since cryptographic functions can take more than a few processor cycles to execute, you really do want to allow node to give itself a chance to handle its I/O before starting on your big chunk of work. Otherwise this library will negatively impact I/O perf much more than is necessary.cyrilchapon commentedon Feb 22, 2019
But, is there a reason such a heavy processing is done synchronously in the first place ?
The fact you remove the fake-callback-still-synchronous strategy is not really addressing that issue.
mcmar commentedon Feb 22, 2019
@cyrilchapon Everything in javascript is inherently single-threaded. Doing it "asynchronously" won't allow any other code to execute simultaneously. It'll just call the callback after at least 1 tick of the event loop (
process.nextTick) and it might also give the event loop time to handle I/O streams (setImmediate)mcmar commentedon Feb 22, 2019
We might be able to use something like threads.js to do this in a real multi-threaded manner, which would be fun and sexy for everyone. But it also might reduce compatibility and/or require pseudo-polyfills like webworker-fallback
mohamedabdelmaksod87 commentedon Mar 12, 2023
I believe the verify method is synchronous in nature and if we considered it a computationally expensive process we may use the node.js worker threads module to run it in parallel out of the main thread from the performance point of view.
The below link shows an alternative popular package to jsonwebtoken experimental (non-blocking) Node.js libuv thread pool-based runtime
#111 (comment)