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

BUG: Large webpack stats bundles cause socket.io client disconnect. #279

Closed
westbrook-asapp opened this issue Mar 29, 2019 · 42 comments · Fixed by #281
Closed

BUG: Large webpack stats bundles cause socket.io client disconnect. #279

westbrook-asapp opened this issue Mar 29, 2019 · 42 comments · Fixed by #281

Comments

@westbrook-asapp
Copy link

westbrook-asapp commented Mar 29, 2019

After upgrading to webpack@4 and webpack-dashboard@3.0.2 I'm able to get a full Success notification as well as Modules/Assets/Problems display. However, when also updating babel to @7, jest to @24, and storybook to @5 (I think only really babel is related, but for completeness) I'm able to build and watch my application seemingly with no issues in the code output, but I'm unable to get the Status to go to Success or the Modules/Assets/Problems sections to display.

If there is something easy that I might be missing, I'd love to hear it, but mostly I'm looking for places in particular to look for issues with my set up here.

====================================================================

If the issue is visual, please provide screenshots here

image

====================================================================

Steps to reproduce the problem

====================================================================

Please provide a gist of relevant files
  1. package.json (specifically the script you are using to start the dashboard)
    ENABLE_SOURCE_MAPS=true NODE_ENV=development webpack-dashboard -- webpack-dev-server --config webpack.config.local-dev.js --host 0.0.0.0 --no-inline
  2. webpack.config.js

====================================================================

More Details
  • What operating system are you on? MacOS
  • What terminal application are you using? Terminal
  • What version of webpack-dashboard are you using? 3.0.2
  • What is the output of running echo $TERM? xterm-256color
@parkerziegler
Copy link
Contributor

Thanks for the issue @westbrook-asapp. I think your intuition about it being Babel-related is probably correct. Do you see the same behavior if you downgrade to Babel 6? Seeing no output like that would lead me to believe that those items (modules, assets, problems) are, for whatever reason, not being sent by inspectpack, or are empty arrays. Is there anyway you can share your setup in a separate repo so I can reproduce locally? If not, I'll see if I can spin up a simple example myself.

@parkerziegler
Copy link
Contributor

@westbrook-asapp so I attempted to reproduce the issue by using creat-react-app to generate a project that uses Babel 7: https://github.com/parkerziegler/webpack-dashboard-babel. The output appears to be normal and the dashboard works as expected, so it's hard to say if Babel is at fault here or not. Without a reproducible example, it's hard for me to debug what might be going on with your setup. Can you take the following steps that might help me diagnose the problem further?

  1. In your node_modules, search for webpack-dashboard.
  2. Within webpack-dashboard, navigate to /dashboard/index.js.
  3. On L171, right after const { assets } = data.value;, add console.log('ASSETS', assets). These are the assets sent by inspectpack, which are used to build out both the "Assets" and "Modules" tables. This will break your dashboard layout because it writes to the dashboard terminal, so don't worry about that. I'm mostly interested in seeing if you get any output there or if its empty.

One last thing I noticed – you seem to pass a --no-inline flag to the dev server, but it doesn't appear that it accepts that flag. Are you attempting to use --inline=false: https://webpack.js.org/configuration/dev-server/#devserverinline?

Again, having a reproducible example you can share is the best way for us to help you. We can't get very far without it unfortunately 😢

@westbrook-asapp
Copy link
Author

@parkerziegler Thanks for your entry analysis of what may be going on here.

Let me take this and see if I can't get closer to the issue today and tomorrow. If not, I'll do the work to sanitize a repro. I attempted this for a previous issue and found out from my team post haste that I hadn't done as good a job as I had hoped, so it may take a little time. I had created a stats out put for that work, so with that and your notes above I should be able to bring something productive back to this issue soon!

@parkerziegler
Copy link
Contributor

Thanks a bunch @westbrook-asapp, great to have your help!

@westbrook-asapp
Copy link
Author

Super weird, it looks like it never even runs this code On L171, right after const { assets } = data.value;, add console.log('ASSETS', assets). trying to debug up the line.

@parkerziegler
Copy link
Contributor

This would probably be the next place to look https://github.com/FormidableLabs/webpack-dashboard/blob/master/dashboard/index.js#L47-L66 to see if those methods are ever getting called.

@westbrook-asapp
Copy link
Author

So, I just ran it with

  setData(dataArray) {
      console.log(dataArray);

And problems comes back, but none of the results are sizes or assets. Continuing up the tree...

@westbrook-asapp
Copy link
Author

westbrook-asapp commented Apr 4, 2019

Once the progress result finally gets to value: 1 it publishes a couple of log results and a problems results, but never a type: 'status', value: 'Complete' or a modules/assets result...

@westbrook-asapp
Copy link
Author

I also tested with webpack-stats-plugin added to my webpack, and it never gets written. Any experience with webpack not getting to the point of emitting a stats object. Reviewing there.

@parkerziegler
Copy link
Contributor

Roping in @ryan-roemer on this one.

@ryan-roemer
Copy link
Member

@westbrook-asapp -- If you remove webpack-dashboard entirely (both CLI and plugin) and keep webpack-stats-plugin does the full build successfully finish?

@westbrook-asapp
Copy link
Author

That's one of the things that's weird, as shown in the image above it always goes to ℹ 「wdm」: Compiled successfully. in the readout, and the code works in the browser, but it's not "Done". I can tell this by the fact that it still doesn't write the stats file.

@ryan-roemer
Copy link
Member

Gotcha. The webpack-stats-plugin is about as simple as a webpack plugin can be. I think we (and you 😉 ) probably need to get to state in which you can generate the output to have a project that could be considered to "minimally handle webpack plugins" first.

... and then maybe after you've reached that state turn back to webpack-dashboard, see if things then work better and touch base with us all here?

Good luck and thanks for all your work!

@westbrook-asapp
Copy link
Author

I'll keep looking at it. Confused because all of the other plugins work, live rebuild works, HappyPack works, ForkTsCheckerWebpackPlugin works, ExtractTextPlugin works, they just never seem to finish. 😖

I'll keep at it. Thanks for helping me thing it through.

@ryan-roemer
Copy link
Member

Can you try with webpack (not webpack-dev-sever) straight up and see if that makes any difference?

Just talking out here -- plugins can hook in to different lifecycle events and that may explain some differences. On our end:

@westbrook-asapp
Copy link
Author

Thanks @ryan-roemer this was actually super helpful. I dug into the events that webpack-dashboard tracks and found that the events that are published by new webpack.ProgressPlugin((percent, msg) => { are overwriting (coming after, though not really if your log them to the console, exploring more) the done event that is listened to in webpackHook(compiler, "done", stats => {. Because of this, even though progress === 1 the UI is overwritten to say "Status: Compiling" even after the Done event has published.

Gonna look into whether new webpack.ProgressPlugin is emitting those events (and their callback) async, which might be causing this. Might be something better there as far as a fix, but does it sound crazy to think a change like the following would be appropriate here:

      handler([
        {
          type: "status",
          value: percent === 1 ? "Done" : "Compiling"
        },
        {
          type: "progress",
          value: percent
        },
        {
          type: "operations",
          value: msg + getTimeMessage(timer)
        }
      ]);

Hopefully, in my search I can find a specific cause as this is just a workaround, but this feels like I'm getting closer.

@westbrook-asapp
Copy link
Author

I was getting the above in /dashboard/index.js not realizing that the plugin was actually running off of /plugin/index.js, so that lead might be a red herring. I'm also not 100% sure how those two files relate, as it seems they're both "running" but it's unclear how...

Separately, I did finally get the StatsWriterPlugin working again, but only when running:

ENABLE_SOURCE_MAPS=true NODE_ENV=development webpack-dashboard -- webpack --config webpack.config.local-dev.js --host 0.0.0.0 --inline=false

as opposed to:

ENABLE_SOURCE_MAPS=true NODE_ENV=development webpack-dashboard -- webpack-dev-server --config webpack.config.local-dev.js --host 0.0.0.0 --inline=false

I've not enough context to judge whether that means this is more likely a webpack, webpack-dev-server, or a webpack-dashboard issue.

The output of the stats is here if reviewing it might help my cause.

@westbrook-asapp
Copy link
Author

Confirmation question, should the handler() calls in https://github.com/FormidableLabs/webpack-dashboard/blob/master/plugin/index.js all be heard in https://github.com/FormidableLabs/webpack-dashboard/blob/master/bin/webpack-dashboard.js and subsequently https://github.com/FormidableLabs/webpack-dashboard/blob/master/dashboard/index.js?

If not, how do the changes in https://github.com/FormidableLabs/webpack-dashboard/blob/master/plugin/index.js make it to https://github.com/FormidableLabs/webpack-dashboard/blob/master/dashboard/index.js? I see a number of calls to states like stats and done in the former and don't see them making their way to the latter and can't find any other path between the two in order to continue debugging this issue.

Thanks in advance for your ongoing support on finding a resolution to this!

@ryan-roemer
Copy link
Member

Correct, as I understand it, the webpack plugin binds webpack events to socket.io handler()-wrapped calls like: https://github.com/FormidableLabs/webpack-dashboard/blob/master/plugin/index.js#L147-L158

    // webpack plugin `failed` hook causes socket.io to emit `status`, `operations` messages
    webpackHook(compiler, "failed", () => {
      handler([
        {
          type: "status",
          value: "Failed"
        },
        {
          type: "operations",
          value: `idle${getTimeMessage(timer)}`
        }
      ]);
    });

Then over in the CLI land, the socket.io listened stuff is punched to: https://github.com/FormidableLabs/webpack-dashboard/blob/master/dashboard/index.js#L104

this.actionForMessageType[data.type](data);

which then uses data.type as a key into this LUT object: https://github.com/FormidableLabs/webpack-dashboard/blob/master/dashboard/index.js#L40-L67

// ... edited ...
    this.actionForMessageType = {
      // ...
      operations: this.setOperations.bind(this),
      status: this.setStatus.bind(this),
      // ...
    };

Does that help? For debugging purposes, you should be able to add console.log before any handler() call and get terminal output to do things like confirm a webpack plugin event fired, etc....

Thanks again for your continued hacking at this!

@westbrook-asapp
Copy link
Author

Ok, so I'm at least a little on the right page. However, what I'm seeing is, for instance this line of code will run https://github.com/FormidableLabs/webpack-dashboard/blob/master/plugin/index.js#L165 (which should demarcate the "Success" display), and I can console.log there. However, I'll never get the follow on functionality at https://github.com/FormidableLabs/webpack-dashboard/blob/master/dashboard/index.js#L135 or its containing method, to be run in response to it. It also appears not to hit https://github.com/FormidableLabs/webpack-dashboard/blob/master/bin/webpack-dashboard.js#L70 though it's super noisy and I might be missing something there.

Any thoughts on why something might not be making it onto the socket?

@westbrook-asapp
Copy link
Author

And, to be clear, it seems to only be the things that happen in webpackHook(compiler, "done", stats => { that get lost. Progress is fine, errors are fine, just done... 😖

@ryan-roemer
Copy link
Member

@westbrook-asapp -- So, just to summarize that I understand what you're saying.

This stuff does get all the way to the CLI calling handler() in the plugin and the CLI responding to it:

webpackHook(compiler, "compile", () => {
webpackHook(compiler, "invalid", () => {
webpackHook(compiler, "failed", () => {

However, this plugin hook is fired and does call handler(), but none of the various events messaged there (e.g., status, progress, stats, etc.) ever reach the CLI end of things:

webpackHook(compiler, "done", stats => {

... and in your experimentation, only the done hook exhibits this behavior.

Did I correctly summarize all the above?

In any case, is there any chance you can give us a minimal reproduction of that? (Ideally a repository or codesandbox with install and command line instructions to reproduce the error...) And I can add in any console.logs that you've manually instrumented...

@westbrook-asapp
Copy link
Author

That is what I am seeing right now, yes. I'll see if I can boil it down to happen somewhere else. 🤞

@westbrook-asapp
Copy link
Author

westbrook-asapp commented Apr 16, 2019

Do you think there is any limit on through put on that socket? In my repro I couldn't recreate, so I focused on specific entry files (again) thinking there might be code somewhere in my app that causes this, but when I focus on only parts of the app it goes to Success... ❓

@ryan-roemer
Copy link
Member

ryan-roemer commented Apr 16, 2019

Let's see if the sheer size of the stats object is causing problems. Can you try something like the following that doesn't send the entire stats object works?

Here's an untested diff that may help get you started:

diff --git a/plugin/index.js b/plugin/index.js
index f90d64e..36ff05a 100644
--- a/plugin/index.js
+++ b/plugin/index.js
@@ -164,6 +164,17 @@ class DashboardPlugin {
         || options.stats
         || { colors: true };
 
+
+      console.log("TODO HERE DONE", JSON.stringify({
+        stats: {
+          // Just get string length instead of actual object.
+          length: JSON.stringify(stats.toJson()).length,
+          errors: stats.hasErrors(),
+          warnings: stats.hasWarnings(),
+          string: stats.toString(statsOptions).length,
+        }
+      }, null, 2));
+
       handler([
         {
           type: "status",
@@ -182,12 +193,12 @@ class DashboardPlugin {
           value: {
             errors: stats.hasErrors(),
             warnings: stats.hasWarnings(),
-            data: stats.toJson()
+            data: {}
           }
         },
         {
           type: "log",
-          value: stats.toString(statsOptions)
+          value: "TODO REMOVED STATS TO STRING"
         }
       ]);
 

@westbrook-asapp
Copy link
Author

westbrook-asapp commented Apr 17, 2019

This seems to be it. With a little extra massaging to the above it does it thing all the way to "Status: Success" as well as all modules/assets in the output (while hard to copy, is it possible to get cursor insertion in the output? I can make a separate ticket) looks like this:

TODO HERE DONE
  "stats":
    "length": 115915534,
    "errors": false,
    "warning": false,
    "string": 10944

😱 🤦‍♂️ Always the absolute last thing in the "maybe it's..." pile!

On one hand, this is exactly why the longer arch of the project I am working on was started (the app was too big and hard to maintain/own), so there's a path towards my team addressing this on our side, but on the other hand, do you think there's a workaround/setting on your part that I could leverage or you could add to support in the sort term?

@ryan-roemer ryan-roemer changed the title Doesn't got to "Status: Success" BUG: Large webpack stats bundles cause socket.io client disconnect. Apr 17, 2019
@ryan-roemer
Copy link
Member

I've confirmed the bug with a simple reproduction placed at: #281 (basically I just pad a single modules source attribute with a huge string and we can get the error.

I've further debugged that it's causing a socket.io disconnection. I'm going to take a break from the issue for a bit, but we all can perhaps look for solutions either in socket.io client, socket.io server, or potentially something super heavyweight like switching to a different means of IPC than socket.io...

@westbrook-asapp
Copy link
Author

Alternate question: What exactly does having the stats object buy a user? The majority of displayed data seems to come from the information sent as sizes, so I wonder if we could just skip sending stats over the socket and just wait for sizes later in the process. Maybe not 100% of the time, but as a work around option?

Spitballing here, but super happy to have something to verify this thing for you and the team. Thanks for helping me get this far!

@ryan-roemer
Copy link
Member

That stats are needed for most of the useful analysis, sizes, duplicates, versions, etc. I'm guessing the likely issue is not the object size per se, but huge module.source fields.

A hacky solution could be to do something like mutate module.source to a good hash so that duplicate detection still work (and sizes are calculated off a separate field value, not the actual source field).

... but if possible, I'd rather solve our IPC problem which I think has a lot of potential avenues...

@westbrook-asapp
Copy link
Author

Makes sense. I only asked because I can't see anything missing right off with this change (short term fork likely on our end 🙈):

            // This can break when you're build it TOOO large.
            // data: stats.toJson()
            // Skip it for now. Yay!
            data: {errors:[],warnings:[]}

Sizes, duplications, modules, assets, etc. all seem to display, but I'm still pretty new to the tool, so thanks for correcting me. Let me know if there is anything else I can do to support a longer term solution here!

@ryan-roemer
Copy link
Member

You know, a "medium-level" thing we could do is to pull inspectpack out of the server / CLI, move it into the client / webpack plugin and do the analysis there, and then send over the JSON results structure for sizes, versions, and duplicates. The final data size for that should be much, much smaller...

I'm not sure if I immediately have time, but I think I could work up something along those lines without too much craziness...

@ryan-roemer
Copy link
Member

WOAH, WAIT A MINUTE. We already do inspectpack on the client / webpack plugin side! @westbrook-asapp fantastic observation -- we're only using warnings|errors and we can easily slice that out of the stats object.

@ryan-roemer
Copy link
Member

Wow @westbrook-asapp that was a super simple fix! Can you pull down and see if #281 works for y'all now? Thanks!

@westbrook-asapp
Copy link
Author

Ha, wish it has been so simple to find! I'll test it out today.

@parkerziegler
Copy link
Contributor

@westbrook-asapp if it does fix your issue I'll publish a patch for it ASAP so your team can get the latest. Thanks @ryan-roemer and @westbrook-asapp for all of your hard work on this – work like this really makes me ❤️ the OSS community.

@westbrook-asapp
Copy link
Author

Working!! 💣 💥 🔥 🚒

Thanks so much to you both for turning this around so quickly.

One note: this code runs, even though it doesn't impede future work:

console.log("Socket.io disconnected.");

It may be nice to make the copy a little more informative (it is also a bit scary if I didn't know why it was happening), but don't let this sort of nit hold things up.

@ryan-roemer
Copy link
Member

@westbrook-asapp -- Great! Good observation about the message.

@parkerziegler -- I'm going to do a little extra work to only log that out if we disconnect _ before_ reaching success state at least once.

@ryan-roemer
Copy link
Member

@westbrook-asapp -- Can you pull the PR one more time, try out c819882 and see if that is a better DX?

@parkerziegler
Copy link
Contributor

@ryan-roemer cool I'll leave it to you to merge and just ping me so I can cut a release.

@westbrook-asapp
Copy link
Author

Much nicer! Let's :shipit: 💯

ryan-roemer added a commit that referenced this issue Apr 17, 2019
…onnect. (#281)

- Limits the webpack stats messaged object to just errors and warnings. Fixes #279 
- Add client disconnect / error messages to help analogous issues in the future.
@westbrook-asapp
Copy link
Author

@parkerziegler Looking forward to a new version this morning. Thanks for everyone's help on this!

@parkerziegler
Copy link
Contributor

@westbrook-asapp latest master released in v3.0.3.

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