Skip to content

Conversation

vdusek
Copy link
Contributor

@vdusek vdusek commented Sep 19, 2025

Description

To align the behavior between the two possible approaches of using an Actor - as a context manager or by explicitly calling init/exit/fail - the following changes were implemented:

  • The Actor's arguments (exit_code, status_message, event_listeners_timeout, cleanup_timeout) were added to the Actor.__init__ as well.
  • exit_code and status_message are now public properties with setters, allowing them to be updated within a context manager block in response to error conditions.
  • The previous approach of calling __aenter__ from init and __aexit__ from exit was reversed, ensuring fewer inconsistencies for future changes (since __aenter__ and __aexit__ have a fixed number of arguments).

Other changes:

  • Related docstrings were updated.
  • The ordering of methods in the Actor was adjusted (see ruff issue #2425).
  • The interface of init, exit, and fail remains unchanged.

Issues

Testing

  • The current test set cover the changes.

@vdusek vdusek self-assigned this Sep 19, 2025
@vdusek vdusek added the t-tooling Issues with this label are in the ownership of the tooling team. label Sep 19, 2025
@github-actions github-actions bot added this to the 123rd sprint - Tooling team milestone Sep 19, 2025
@github-actions github-actions bot added the tested Temporary label used only programatically for some analytics. label Sep 19, 2025
@vdusek vdusek force-pushed the init-exit branch 2 times, most recently from fcec3b9 to 3dacd65 Compare September 19, 2025 16:05
@Pijukatel
Copy link
Contributor

Am not sure about this. What about this scenario?

...
async with Actor(...):
    await Actor.fail(exit_code=1, status_message='This is a test message')

This used to work before, but now it will try to call __aexit__ twice

@vdusek
Copy link
Contributor Author

vdusek commented Sep 22, 2025

IMO that scenario is a misuse. You've got 2 approaches and you should choose one of them. Not combining them.

@B4nan
Copy link
Member

B4nan commented Sep 22, 2025

People can use Actor.fail (as well as Actor.exit) as part of their code, even in the request handler, so I wouldn't call that a misuse really. They might want to quit prematurely on purpose, the method is not always supposed to be at the very end of the main script. In JS, it does process.exit so it quits the process completely, so you won't fall into this, the top level Actor.exit wouldn't be called if that happens.

Can we keep a flag to know the exit method was already called, and skip the duplicated logic that would be otherwise produced by the context manager?

@Pijukatel
Copy link
Contributor

I added one test. We can decide whether it is an allowed use case that should be supported or a misuse.
a03b3f1

Copy link
Contributor

@janbuchar janbuchar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vdusek
Copy link
Contributor Author

vdusek commented Oct 9, 2025

If you mean this:

Am not sure about this. What about this scenario?

...
async with Actor(...):
    await Actor.fail(exit_code=1, status_message='This is a test message')

This used to work before, but now it will try to call aexit twice

Then yes, this is a valid scenario (we have the _is_exiting flag).

@vdusek vdusek changed the title refactor: Unify Actor context manager with init & exit methods fix: Unify Actor context manager with init & exit methods Oct 10, 2025
run_result = await run_actor(actor)

assert run_result.exit_code == 91
assert run_result.exit_code == ActorExitCodes.ERROR_USER_FUNCTION_THREW.value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually prefer "inlining" the constants to avoid tautological tests. Not a hill to die on in this case, just saying

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unify Actor's context manager with manual init/exit methods

4 participants