jwt.verify() not truly async.

[vicary]

Despite the verify action is in fact synchronize, the callback API does not.

It is specifically this line that released Zalgo.

node-jsonwebtoken/verify.js

Line 24 in 5a7fa23

done = callback;

Which can be fixed by wrapping it as such

    done = function(err, data) {
      setImmediate(callback, err, data);
    };

[ziluvatar]

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]

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]

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]

@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]

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.nextTick to setImmediate. You can read about the differences here: https://nodejs.org/en/docs/guides/event-loop-timers-and-nexttick/

TL;DR process.nextTick doesn'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]

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]

@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]

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]

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)