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

The syntax around inflight init and inflight fields is misleading #4564

Open
eladb opened this issue Oct 17, 2023 · 10 comments
Open

The syntax around inflight init and inflight fields is misleading #4564

eladb opened this issue Oct 17, 2023 · 10 comments
Labels
🐛 bug Something isn't working 📐 language-design Language architecture needs-discussion Further discussion is needed prior to impl 📜 spec Requires a change in the Language/SDK spec

Comments

@eladb
Copy link
Contributor

eladb commented Oct 17, 2023

I tried this:

Consider this example:

bring cloud;

class MyService {
  b: cloud.Bucket;
  init() {
    this.b = new cloud.Bucket();
  }
  inflight init() {
    this.b.put("file.txt", "initial message");
  }
  pub inflight set(message: str) {
    this.b.put("file.txt", message);
  }
  pub inflight get(): str {
    return this.b.get("file.txt");
  }
}

let s = new MyService();

let f1 = new cloud.Function(inflight () => {
  s.set("hello");
}) as "f1";

let f2 = new cloud.Function(inflight (): str => {
  return s.get();
}) as "f2";

test "test" {
  f1.invoke("");
  let value = f2.invoke("");
  log(value);
}

A similar issue exists with inflight fields:

class Foo {
  inflight n: num;
  inflight init() {
    this.n = 10;
  }
  pub inflight inc() {
    this.n = this.n + 1;
  }
}

In the above example, n will be 10 for every client.

See #4561 (comment)

This happened:

You might expect that after invoking f1, then f2 will return "hello", but it returns "initial message".

This is because the inflight initializer is called once per client.

I expected this:

The purpose of inflight initializers and fields is to allow caching state within the inflight client of a resource.

Is there a workaround?

No response

Component

Language Design

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.
@eladb eladb added the 🐛 bug Something isn't working label Oct 17, 2023
@eladb eladb mentioned this issue Oct 17, 2023
5 tasks
@staycoolcall911 staycoolcall911 added 📐 language-design Language architecture needs-discussion Further discussion is needed prior to impl 📜 spec Requires a change in the Language/SDK spec labels Oct 19, 2023
@eladb
Copy link
Contributor Author

eladb commented Dec 7, 2023

Use case for inflight init and fields: winglang/winglibs#6 (review)

having second thoughts about getting rid of this feature..

(maybe we can brainstorm on alternative syntaxes first...)

@staycoolcall911
Copy link
Contributor

Use case for inflight init and fields: winglang/winglibs#6 (review)

I think that's the use case @yoav-steinberg was also referring to.

@eladb
Copy link
Contributor Author

eladb commented Dec 7, 2023

yes, of course, that's the use case.

@yoav-steinberg
Copy link
Contributor

(maybe we can brainstorm on alternative syntaxes first...)

See #3174 (comment)

@eladb
Copy link
Contributor Author

eladb commented Dec 7, 2023

@yoav-steinberg I am not sure I understand how lift helps in this context. Can you provide an example of how this would be written with this proposed syntax?

@yoav-steinberg
Copy link
Contributor

yoav-steinberg commented Dec 8, 2023

@eladb Sorry for the long answer:
The way I see it, technically we're fine with what we have now, the problem is that the syntax is misleading. Why is it misleading? Because there's code that's implicitly executed during a lift (the inflight new()) and because there's client state that isn't synced between clients (the inflight fields) and it's unclear to the user whether it's synced or not.

Now you can ask yourself, why isn't this a problem in regular OOP? If you have a class with static fields and some static field initialization mechanism and you have a mechanism of instantiating class instances (new) then you basically have the same problem:
You can modify a this.instance_field and expect it to be modified in all instances. You also have ctor code that can modify some global state and two calls to new might overwrite the global state surprising the user.

So what's the difference? The answer is: explicit instantiation. When you call new X() you know you're running ctor code. You also assign the result into a variable so you know you just created a new per instance state:

// In the following code I know I'm running Foo's ctor.
// In the following code I know I'm creating a new distinct, non-shared, non-static state inside the variable x.
let x = new Foo();

So now we want to translate that experience to lifting:

  • We need it to be explicit so we know that the inflight ctor is being executed.
  • We need it to define a new inflight state instance (client state) so we need some assignment to a "client" variable.

Here's the redis client example:

class Redis_sim {
  inflight client: IRedisClient;
  pub inflight client_cmd_count: num;
  inflight lift() { // `lift` is the inflight equivalent to `new`, we still enforce `inflight` modifier for clarity 
    this.client = utils.newClient(this.connectionUrl);
    this.client_cmd_count = 0;
  }

  pub inflight get(key: str): str? {
    this.client_cmd_count += 1;
    return this.client.get(key);
  }

  // ...
}

let rsim = new Redis_sim();
let some_inflight = inflight () => {
  let rclient = lift Redis_sim() from rsim; // Explicit lifting required, i used the most verbose syntax but we can have sugar
  rclient.get("key");
  log("{rclient.client_cmd_count}");
}

@staycoolcall911 staycoolcall911 added this to the Wing Cloud 1.0 milestone Dec 10, 2023
@staycoolcall911
Copy link
Contributor

Great explanation, thanks @yoav-steinberg.
I also wanted to suggest keeping the call to inflight c'tor implicit, as it is today, but renaming from inflight new() to inflight onLift() - this way we communicate to the user that this is a lifecycle method called every time an object is lifted from preflight to inflight.

Copy link

github-actions bot commented Feb 9, 2024

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!

@yoav-steinberg
Copy link
Contributor

I'm leaning towards @staycoolcall911's suggestion to just be a rename to something like onLift. It's a slight improvement in understandability without changing how things work today. Lets have a final discussion and try to close this one.

@eladb
Copy link
Contributor Author

eladb commented Apr 7, 2024

I am also comfortable not dealing with this at the moment. Maybe we can use some more experience with using these idioms before we change things.

@staycoolcall911 staycoolcall911 removed this from the Winglang Stable Release milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 📐 language-design Language architecture needs-discussion Further discussion is needed prior to impl 📜 spec Requires a change in the Language/SDK spec
Projects
Status: 🤝 Backlog - handoff to owners
Development

No branches or pull requests

4 participants