Skip to content
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

Callback was already called. #542

Closed
mariusrak opened this issue May 23, 2020 · 10 comments · Fixed by #681
Closed

Callback was already called. #542

mariusrak opened this issue May 23, 2020 · 10 comments · Fixed by #681
Labels

Comments

@mariusrak
Copy link

Hi, I'm using memfs with webpack and its watcher. After rebuild I want to read a file that was just built, but get this error:

    at throwError (C:\...\node_modules\neo-async\async.js:16:11)
    at C:\...\node_modules\neo-async\async.js:2818:7
    at C:\...\node_modules\neo-async\async.js:2830:7
    at done (C:\...\node_modules\neo-async\async.js:3517:9)
    at C:\...\node_modules\webpack\lib\Compiler.js:548:25
    at Immediate.<anonymous> (C:\...\node_modules\memfs\lib\volume.js:684:17)
    at processImmediate (internal/timers.js:456:21)```
@G-Rath
Copy link
Collaborator

G-Rath commented May 23, 2020

Could you provide some more information about your setup and how you're trying to do what you're trying to do? (A reproduction repo would ideal, but otherwise some code would also be useful)

Assuming you're using the latest version of memfs, memfs\lib\volume.js:684:17 holds var node = child.getNode(); in _toJSON, meaning something is asking memfs to render the state of itself in JSON form and it's erroring for some reason.

Also if you're not depending on memfs directly, it would be useful to know what's bring it in:

  • if you're using npm, could you grab the output of npm ls memfs?
  • if you're using yarn could you grab the output of yarn why memfs?

(I'm not too familiar with the dependency trees of webpack, outside of knowing it does use memfs, so that would save me having to open a bunch of package.jsons on github)

@mariusrak
Copy link
Author

mariusrak commented May 26, 2020

Here's the important part of my code. But I found out, that I have some other error, which comes up on webpack rebuild. And it seems that while there's no error in memfs, it blocks displaying actual user-code error. So the issue here would be, why memfs is hiding the error.

const path = require("path");
const watchpack = require("watchpack");

const { createFsFromVolume, Volume } = require("memfs");
const fs = createFsFromVolume(new Volume());

const webpack = require("webpack");

const compiler = webpack([
        {
                entry: path.resolve(__dirname, "./file.js"),
                output: { path: path.resolve(__dirname, "dist"), filename: "file.js" },
                mode: "development",
        },
]);
compiler.outputFileSystem = fs;
const WatchHandler = compiler.watch(
        {
                aggregateTimeout: 300,
                poll: undefined,
        },
        (err, stats) => {
                if (err) {
                        console.log(err);
                } else if (stats.hasErrors()) {
                        console.log(stats.toString({ colors: true }));
                } else {
                        // I have error here, so e.g.
                        throw 'error';
                }
        }
);

But using try - catch in the callback helps.

@G-Rath
Copy link
Collaborator

G-Rath commented Jun 6, 2020

@mariusrak would you be able to put together a repo reproducing this behaviour that I could use to better investigate?

@bpatath
Copy link

bpatath commented Sep 27, 2020

I got this issue too. I think it comes from an error in the Volume.wrapAsync method. Currently, the try block contains both calls to method and callback. Thus, if it is the callback which raises an exception, the callback will be called a second time in the catch block and the Callback was already called. will happen.

Calling the callback should probably be a separated step ensuring that the callback is only called once, e.g.:

let v = null, e = null;
try {
  v = method.apply(...);
} catch (err) {
  e = err;
}
callback(e, v);

@G-Rath
Copy link
Collaborator

G-Rath commented Oct 17, 2020

@bpatath @mariusrak would either of you be able to provide a sample repo reproducing this problem so that I can investigate?

I'm happy to look into this, but have a heavy workload atm that means I don't really have the bandwidth to invest into figuring out the setup needed to reproduce - having a repo that I can just npm install && npm run do-the-thing would make it much easier for me to action :)

(it doesn't have to be just one command, - having a repo with all the right code & configs, + the steps to create the error is the main thing)

@AvalonCV
Copy link

Hello everyone,

sorry if I jump into this, but I recently encountered the same error. @G-Rath I uploaded my broken code to https://github.com/AvalonCV/p2020 . npm install && npm run dev-server should lead you to the problem (that I was able to fix using the changes proposed by @bpatath to find the actual error).

Best regards

@G-Rath
Copy link
Collaborator

G-Rath commented Oct 26, 2020

Ah, I think I see what's happenings - the callback itself is throwing an error, that must happen after the callback internally registers itself as called.

That's a good catch - I'll try and have a PR up tonight fixing it :)

@AvalonCV
Copy link

Great. Thank you very much :)

@privatenumber
Copy link
Contributor

Hope it's alright if I work on a fix for this

streamich pushed a commit that referenced this issue Aug 31, 2021
## [3.2.3](v3.2.2...v3.2.3) (2021-08-31)

### Bug Fixes

* global and timers this arg in browser ([1e93ab1](1e93ab1))
* prevent callback from triggering twice when callback throws ([07e8215](07e8215))
* prevent callback from triggering twice when callback throws ([6db755d](6db755d)), closes [#542](#542)
@streamich
Copy link
Owner

🎉 This issue has been resolved in version 3.2.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants