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

running multiple compilations simultaneously in the same node process #3678

Closed
polamoros opened this issue Aug 2, 2023 · 11 comments · Fixed by #5247
Closed

running multiple compilations simultaneously in the same node process #3678

polamoros opened this issue Aug 2, 2023 · 11 comments · Fixed by #5247
Assignees
Labels
🐛 bug Something isn't working 🛠️ compiler Compiler

Comments

@polamoros
Copy link
Contributor

polamoros commented Aug 2, 2023

I tried this:

import path from "path";

import * as wing from "@winglang/compiler";

const wingfile1 = path.resolve("file1.w");
const wingfile2 = path.resolve("file2.w");

const compile = async (wingfile: string) => {
  const simfile = await wing.compile(wingfile, {
    target: wing.Target.SIM,
  });
  return simfile;
};

const testCompile = async () => {
  compile(wingfile1);
  compile(wingfile2);
  compile(wingfile1);
  compile(wingfile2);
};

testCompile();

file1.w

bring cloud;

let bucket = new cloud.Bucket() as "test1_bucket";
let queue = new cloud.Queue() as "test1_queue";
let api = new cloud.Api() as "test1_api";

file2.w

bring cloud;

let bucket = new cloud.Bucket() as "test2_bucket";

This happened:

the target/file2.wsim to contain the file1 compilations files.
image

target.zip

I expected this:

The compiler to compile the correct file.

Is there a workaround?

no

Component

Compiler

Wing Version

No response

Node.js Version

No response

Platform(s)

No response

Anything else?

No response

Community Notes

  • Please vote by adding a 👍 reaction to the issue to help us prioritize.
  • If you are interested to work on this issue, please leave a comment.
@polamoros polamoros added the 🐛 bug Something isn't working label Aug 2, 2023
@skyrpex
Copy link
Contributor

skyrpex commented Aug 2, 2023

It doesn't happen if both consoles are created in different node processes.

I'm guessing there's a singleton-like functionality somewhere in the Wing SDK / Simulator... @Chriscbr Anything on top of your head that may be related to this issue?

@ainvoner
Copy link
Contributor

ainvoner commented Aug 2, 2023

in libs/wingcompiler/src/compile.ts there are several process.env usages and assignments. Since the compile command is async, we might overriding those values when running multiple compile commands rapidly in the same process?

@polamoros polamoros changed the title runing multiple simulator instances in the same node process running multiple compilations simultaneously in the same node process Aug 2, 2023
@skyrpex
Copy link
Contributor

skyrpex commented Aug 2, 2023

We should be able to pass the necessary info using normal option objects instead of using process.env, right?

@polamoros polamoros added the 🛠️ compiler Compiler label Aug 2, 2023
@Chriscbr
Copy link
Contributor

Chriscbr commented Aug 2, 2023

@skyrpex @polamoros Could you create a repro that only uses @winglang/sdk or @winglang/compiler?

@polamoros
Copy link
Contributor Author

Hey @Chriscbr, I tried calling the compiler multiple times with two different files, and it caused the target directory of file2 to contain the output of compiling file1.

(I'll upload the test I did later today)

@polamoros
Copy link
Contributor Author

@Chriscbr I updated the issue description with the test I did using the compiler only.

@ekeren
Copy link
Collaborator

ekeren commented Aug 3, 2023

@polamoros, do you mind expressing the urgency here, I want to understand the priority here?

@polamoros
Copy link
Contributor Author

@ekeren is not really urgent because we found a workaround and we were able to release the vs-code extension anyways.

It would be nice to fix it to be able to use the wingconsole/app package to run the console inside the vs-code extension instead of running a different node process for each console instance and also improving the integration between them.

@github-actions
Copy link

github-actions bot commented Oct 4, 2023

Hi,

This issue hasn't seen activity in 60 days. Therefore, we are marking this issue as stale for now. It will be closed after 7 days.
Feel free to re-open this issue when there's an update or relevant information to be added.
Thanks!

Copy link

Hi,

This issue hasn't seen activity in 60 days. Therefore, we are marking this issue as stale for now. It will be closed after 7 days.
Feel free to re-open this issue when there's an update or relevant information to be added.
Thanks!

@github-actions github-actions bot added the Stale label Dec 16, 2023
@MarkMcCulloh MarkMcCulloh self-assigned this Dec 19, 2023
@mergify mergify bot closed this as completed in #5247 Dec 20, 2023
mergify bot pushed a commit that referenced this issue Dec 20, 2023
…5247)

#### General Vibe
- This is an MVP of getting TS working via the wing cli (`compile`, `run`, and `test`). This is basically only usable for preflight
- I wanted to change as little as possible in the SDK to avoid wasted work once resources are moved to winglibs
- Does not address inflight stuff yet

The entrypoint TS experience is different than the existing issue/story:

```ts
import { main } from "ts4wing";

main((app) => {
  // stuff goes here
})
```

This was due to our usage of a "root" construct for testing isolation. The wing cli needs to have control over the instantiation of resources, so `new App()` is not really feasible because we don't want users to actually attach stuff at the app-level. It may be possible to get it working with `new App()` but the hacks needed didn't seem worth it.
An added benefit of this approach is that the user no longer needs to call `app.synth()`

Anecdotally, I have built an awscdk-based internal framework at a previous job that looked like this for a similar reason (to duplicate stuff across Stages) and it worked pretty well.

#### Indirect Changes
- Made sure everything matches their `@types/node` version
- **preflight execution now happens in a worker thread instead of a VM**
  - VM context did not allow data to be provided past the initially loaded file.
  - In a VM, process.env is useful but pollutes the parent process
  - The overloaded "require" was only useful for the initial file. Now I added a shim that overloads require everywhere to make sure the SDK is loaded properly

Fixes #3678

*By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*.
@monadabot
Copy link
Contributor

Congrats! 🚀 This was released in Wing 0.53.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🛠️ compiler Compiler
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

8 participants