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

cloud.Service causes thread to hang #4086

Closed
Chriscbr opened this issue Sep 5, 2023 · 5 comments · Fixed by #4089
Closed

cloud.Service causes thread to hang #4086

Chriscbr opened this issue Sep 5, 2023 · 5 comments · Fixed by #4089
Assignees
Labels
🐛 bug Something isn't working 🎨 sdk SDK

Comments

@Chriscbr
Copy link
Contributor

Chriscbr commented Sep 5, 2023

I tried this:

I wrote this Wing program:

bring cloud;
bring util;

let counter = new cloud.Counter();

let service = new cloud.Service(onStart: inflight () => {
  while true {
    if counter.peek() > 0 {
      counter.inc();
      return;
    }
    util.sleep(0.1s);
  }
});

test "counter increases by 2" {
  assert(counter.peek() == 0);
  counter.inc();
  util.sleep(1s);
  assert(counter.peek() == 2);
}

and I ran wing test main.w

This happened:

The test hangs indefinitely

I expected this:

I expected the test to pass. cloud.Service represents a long-running service, so it should be possible to have it run code continuously in the background.

Is there a workaround?

No response

Component

SDK

Wing Version

0.28.1

Node.js Version

18.14.1

Platform(s)

MacOS

Anything else?

Even though the simulator is single-threaded, this shouldn't cause an issue since the service's onStart code calls util.sleep() which gives the JavaScript runtime the chance switch to other tasks.

See this passing test with cloud.Function:

bring cloud;
bring util;

let queue = new cloud.Queue();
let counter = new cloud.Counter();

let fn = new cloud.Function(inflight () => {
  while true {
    if counter.peek() > 0 {
      counter.inc();
      return;
    }
    util.sleep(0.1s);
  }
});
queue.setConsumer(inflight () => {
  fn.invoke("");
});

test "counter increases by 2" {
  assert(counter.peek() == 0);
  queue.push("");
  counter.inc();
  util.sleep(1s);
  assert(counter.peek() == 2);
}

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.
@Chriscbr Chriscbr added 🐛 bug Something isn't working 🎨 sdk SDK labels Sep 5, 2023
@Chriscbr Chriscbr self-assigned this Sep 5, 2023
@mergify mergify bot closed this as completed in #4089 Sep 7, 2023
mergify bot pushed a commit that referenced this issue Sep 7, 2023
Fixes #4086

## Checklist

- [x] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted)
- [x] Description explains motivation and solution
- [x] Tests added (always)
- [ ] Docs updated (only required for features)
- [ ] Added `pr/e2e-full` label if this feature requires end-to-end testing

*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.29.7.

@eladb
Copy link
Contributor

eladb commented Sep 22, 2023

@Chriscbr this feels wrong (at least in my mind). The intent was that onStart is asynchronous and never blocks.

Maybe we need to change its name or document it better but as I see it onStart should kick off the start of the server and yield asynchronously. A while(true) model is less common in Node runtimes.

@Chriscbr
Copy link
Contributor Author

@eladb OK, got it. Definitely sounds worth documenting.

as I see it onStart should kick off the start of the server and yield asynchronously

right, if I understand your comment correctly, if I wanted to achieve the behavior in the example I gave, I'd have to move the while loop into an inflight function like startServer, and then call defer startServer(); inside of onStart so that it doesn't block things? Yes I think this should work 👍

@eladb
Copy link
Contributor

eladb commented Sep 24, 2023

I'd have to move the while loop

Not sure exactly what's the use case behind the example, but generally speaking, busy-waiting is not a common pattern in node.js based programs. Perhaps some kind of an async interval is the right fit for something like this (setInterval(), which we don't support in util yet I believe).

@Chriscbr
Copy link
Contributor Author

Chriscbr commented Sep 24, 2023

Not sure exactly what's the use case behind the example

Say if you were trying to poll for tasks from an SQS queue from an ECS task container (you aren't a built-in AWS integration, so you are responsible for doing polling yourself). If I was implementing this, my initial hunch would be to write a busy loop of some kind, right?

  1. Try to receive a batch of messages
  2. If messages were received, process them, go back to step 1
  3. If there were no messages, sleep for a bit, go back to step 1

But I think you're right that using setInterval() would be more idiomatic in the Node.js world, and it feels like a better way to solve this...

mergify bot pushed a commit that referenced this issue Sep 26, 2023
A missing `await` in the simulator `start()` method resulted in resource becoming ready prematurely (fixes #4251).

This reverts the change in #4086 which mistakingly assumed that `onStart` can block. Added a comment to the documentation of this handler.


## Checklist

- [x] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted)
- [x] Description explains motivation and solution
- [x] Tests added (always)
- [x] Docs updated (only required for features)
- [x] Added `pr/e2e-full` label if this feature requires end-to-end testing

*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)*.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🎨 sdk SDK
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants