workers-rs icon indicating copy to clipboard operation
workers-rs copied to clipboard

[BUG] Version 0.0.10 causes `wrangler publish` failure

Open awaitlink opened this issue 3 years ago • 16 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues

What version of workers-rs are you using?

0.0.10

Describe the bug

With workers-rs 0.0.10, deploying u32i64/signalupdates-bot (e.g. at this revision) fails with the following error:

 ⛅️ wrangler 2.0.15 
--------------------
Running custom build: cargo install worker-build && worker-build --release
    Updating crates.io index
     Ignored package `worker-build v0.0.6` is already installed, use --force to override
[INFO]: 🎯  Checking for the Wasm target...
[INFO]: 🌀  Compiling to Wasm...
    Finished release [optimized] target(s) in 0.07s
[INFO]: ⬇️  Installing wasm-bindgen...
[INFO]: Optimizing wasm binaries with `wasm-opt`...
[INFO]: Optional fields missing from Cargo.toml: 'description', 'repository', and 'license'. These are not necessary, but recommended
[INFO]: ✨   Done in 1.24s
[INFO]: 📦   Your wasm pkg is ready to publish at [redacted path]/signalupdates-bot/build.
Your worker has access to the following bindings:
- KV Namespaces:
  - STATE: [redacted namespace id]

✘ [ERROR] A request to the Cloudflare API (/accounts/[redacted account id]/workers/scripts/signalupdates-bot) failed.

  Uncaught TypeError: Cannot read properties of undefined (reading
  'memory')
    at line 0
   [code: 10021]

  
  If you think this is a bug, please open an issue at:
  https://github.com/cloudflare/wrangler2/issues/new/choose

Deploying the same worker with workers-rs 0.0.9 (e.g. at this revision) is successful (hence I'm reporting this here instead of cloudflare/wrangler2 as the error suggests).

The wrangler.toml file is identical to wrangler.example.toml, except the account and KV namespace IDs are specified instead of ....

Steps To Reproduce

No response

awaitlink avatar Jun 22 '22 17:06 awaitlink

Hmmm, it seems to be that wasm-bindgen doesn't always generate the same definition of cachedFloat64Memory0 and similar variables causing the modification we do to the generated JS glue after the fact to break. Building worker-sandbox has these set to null whereas your worker assigns them to new Float64Array(wasm.memory.buffer).

worker-build currently works under the assumption that nothing in the JS glue tries to access the wasm variable until the WASM is actually invoked, but apparently that isn't always the case. I'll work on a fix but you should probably roll back to 0.0.9 and pin your worker-build install command to the previous version.

zebp avatar Jun 23 '22 21:06 zebp

I've identified a fix but it's currently blocked on https://github.com/swc-project/swc/issues/4864. This puts us in a weird place where we ideally want to keep using SWC, but it looks like it'll be a while until it's fixed. Whereas esbuild doesn't suffer from this behavior but would require either esbuild or npm on the user's path.

I also identified a hacky fix that would fix this issue, but another one similar to this could pop up later whenever wasm-bindgen makes changes to the generated JS. I think that's what we'll have to go with for now.

zebp avatar Jun 24 '22 17:06 zebp

Whereas esbuild doesn't suffer from this behavior but would require either esbuild or npm on the user's path.

If this SWC doesn't fix this in the future, we could actually just download the esbuild binary from the npm package and store it in the user's cache dir if it isn't already on the user's path.

zebp avatar Jun 24 '22 17:06 zebp

I created a branch that uses (or downloads) esbuild for the bundling step to hopefully solve this issue. I'm not sure this is the path we want to go down yet, so for now replace your build.command with the TOML below if you'd like to try it out. This did allow me to publish your worker.

[build]
command = "cargo install -q --git https://github.com/cloudflare/workers-rs --branch zeb/esbuild && worker-build"

zebp avatar Jun 24 '22 20:06 zebp

I have the same issue when running wrangler dev as well

tsunrise avatar Jun 25 '22 06:06 tsunrise

@zebp Thanks for working on this!

There appears to be a typo in darwin that prevents esbuild from being downloaded on Apple silicon:

https://github.com/cloudflare/workers-rs/blob/3fae93d1387b6e0694f82d73059ffae3de49524b/worker-build/src/install.rs#L127

I've chosen to install esbuild manually using yarn global add esbuild, then the worker deploys and works successfully using your branch with workers-rs 0.0.10.

awaitlink avatar Jun 25 '22 09:06 awaitlink

@zebp Thanks for working on this!

There appears to be a typo in darwin that prevents esbuild from being downloaded on Apple silicon:

https://github.com/cloudflare/workers-rs/blob/3fae93d1387b6e0694f82d73059ffae3de49524b/worker-build/src/install.rs#L127

I've chosen to install esbuild manually using yarn global add esbuild, then the worker deploys and works successfully using your branch with workers-rs 0.0.10.

Fixed the typo, glad that it works for you!

zebp avatar Jun 25 '22 12:06 zebp

On Windows, the esbuild branch fails to install with this error:

error[E0433]: failed to resolve: could not find `unix` in `os`
  --> worker-build\src\install.rs:74:22
   |
74 |         use std::os::unix::prelude::OpenOptionsExt;
   |                      ^^^^ could not find `unix` in `os`

error[E0599]: no method named `mode` found for mutable reference `&mut OpenOptions` in the current scope
  --> worker-build\src\install.rs:75:27
   |
75 |         options = options.mode(0o770);
   |                           ^^^^ method not found in `&mut OpenOptions`

fluxehub avatar Jul 04 '22 20:07 fluxehub

On Windows, the esbuild branch fails to install with this error:

error[E0433]: failed to resolve: could not find `unix` in `os`
  --> worker-build\src\install.rs:74:22
   |
74 |         use std::os::unix::prelude::OpenOptionsExt;
   |                      ^^^^ could not find `unix` in `os`

error[E0599]: no method named `mode` found for mutable reference `&mut OpenOptions` in the current scope
  --> worker-build\src\install.rs:75:27
   |
75 |         options = options.mode(0o770);
   |                           ^^^^ method not found in `&mut OpenOptions`

Whoops! The branch has been updated with a fix.

zebp avatar Jul 05 '22 12:07 zebp

esbuild doesn't seem to like the way I structured my workspace. When I apply @zebp's workaround I get

$ miniflare
    Updating git repository `https://github.com/cloudflare/workers-rs`
     Ignored package `worker-build v0.0.6 (https://github.com/cloudflare/workers-rs?branch=zeb/esbuild#ec5546ec)` is already installed, use --force to override
[INFO]: 🎯  Checking for the Wasm target...
[INFO]: 🌀  Compiling to Wasm...
warning: profiles for the non root package will be ignored, specify profiles at the workspace root:
package:   /Users/chris/repos/github.com/cloudflare/daphne/daphne_worker_test/Cargo.toml
workspace: /Users/chris/repos/github.com/cloudflare/daphne/Cargo.toml
    Finished release [optimized] target(s) in 0.18s
[INFO]: License key is set in Cargo.toml but no LICENSE file(s) were found; Please add the LICENSE file(s) to your project directory
[INFO]: ⬇️  Installing wasm-bindgen...
[INFO]: Optimizing wasm binaries with `wasm-opt`...
[INFO]: Optional fields missing from Cargo.toml: 'description', 'repository'. These are not necessary, but recommended
[INFO]: ✨   Done in 2.24s
[INFO]: 📦   Your wasm pkg is ready to publish at /Users/chris/repos/github.com/cloudflare/daphne/daphne_worker_test/build.

  shim.mjs  43.9kb

⚡ Done in 8ms
[mf:inf] Build succeeded
[mf:err] DurableObjectError [ERR_CLASS_NOT_FOUND]: Class "ReportStore" for Durable Object "DAP_REPORT_STORE" not found
    at DurableObjectsPlugin.reload (/Users/chris/.nvm/versions/node/v17.1.0/lib/node_modules/miniflare/node_modules/@miniflare/durable-objects/src/plugin.ts:255:15)
    at EventTarget.#runAllReloads (/Users/chris/.nvm/versions/node/v17.1.0/lib/node_modules/miniflare/node_modules/@miniflare/core/src/index.ts:656:24)
    at EventTarget.#reload (/Users/chris/.nvm/versions/node/v17.1.0/lib/node_modules/miniflare/node_modules/@miniflare/core/src/index.ts:797:13)
    at EventTarget.getPlugins (/Users/chris/.nvm/versions/node/v17.1.0/lib/node_modules/miniflare/node_modules/@miniflare/core/src/index.ts:970:5)
    at createServer (/Users/chris/.nvm/versions/node/v17.1.0/lib/node_modules/miniflare/node_modules/@miniflare/http-server/src/index.ts:350:19)
    at startServer (/Users/chris/.nvm/versions/node/v17.1.0/lib/node_modules/miniflare/node_modules/@miniflare/http-server/src/index.ts:436:18)
    at main (/Users/chris/.nvm/versions/node/v17.1.0/lib/node_modules/miniflare/src/cli.ts:96:5)

I'm using miniflare version 2.5.1. I get the same error with wrangler publish.

The worker is in a crate daphne_worker_test. The class that's missing, ReportStore, is in a crate daphne_worker in the same workspace. It appears this is a problem for esbuild, but not SWC. Any ideas for working around this?

Here's my repo: https://github.com/cloudflare/daphne/compare/cjpatton/wrangler-build

cjpatton avatar Jul 14 '22 01:07 cjpatton

Looks like the blocking issue may have been resolved? https://github.com/swc-project/swc/issues/4864 I will look today or tomorrow to see if upgrading SWC solves the issue.

cjpatton avatar Jul 14 '22 14:07 cjpatton

Seems that upgrading didn't immediately help, sadly :(

cjpatton avatar Jul 14 '22 21:07 cjpatton

After a bit of fiddling, I don't think the current workaround is working for durable objects at all. Has anyone gotten this to work with durable objects?

cjpatton avatar Jul 14 '22 21:07 cjpatton

@zebp pushed a change to the workaround branch that fixed my DO issue 👍

cjpatton avatar Jul 16 '22 00:07 cjpatton

@zebp after switching to using worker-build from this repo's main branch, we now see a different error:

✘ [ERROR] A request to the Cloudflare API (…) failed.

  Uncaught Error: LinkError: WebAssembly.Instance(): Import #47 module="./index_bg.js" function="__wbg_clearTimeout_65417660fe82f08d" error: function import requires a callable
    at line 0
   [code: 10021]

  
  If you think this is a bug, please open an issue at: https://github.com/cloudflare/wrangler2/issues/new/choose

Are we doing something wrong? It looks like the binding to the clearTimeout() implementation is somehow missing from the final shim.mjs.

jakubadamw avatar Jul 30 '22 18:07 jakubadamw

@zebp after switching to using worker-build from this repo's main branch, we now see a different error:

✘ [ERROR] A request to the Cloudflare API (…) failed.

  Uncaught Error: LinkError: WebAssembly.Instance(): Import #47 module="./index_bg.js" function="__wbg_clearTimeout_65417660fe82f08d" error: function import requires a callable
    at line 0
   [code: 10021]

  
  If you think this is a bug, please open an issue at: https://github.com/cloudflare/wrangler2/issues/new/choose

Are we doing something wrong? It looks like the binding to the clearTimeout() implementation is somehow missing from the final shim.mjs.

Guess some new bugs emerged switching to esbuild, I'm trying to reproduce using Delay but I haven't gotten that issue. I also haven't been able to reproduce your issue with manual calls to clear_timeout.

Are you able to reduce it down to a small reproducable worker you can share?

zebp avatar Aug 01 '22 13:08 zebp

@zebp pardon, I was not able to reduce it down easily, but somehow it went through at another attempt. We're now using workers 0.0.10 just fine.

jakubadamw avatar Aug 12 '22 14:08 jakubadamw

@zebp pardon, I was not able to reduce it down easily, but somehow it went through at another attempt. We're now using workers 0.0.10 just fine.

Good, although it just working a second time is a bit concerning. I think I'm going to close this issue, if any issues come up with the new bundler then a new issue would probably be better.

zebp avatar Aug 12 '22 20:08 zebp