Skip to content

feat: allow custom exit function through dependency injection #389

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

feat: allow custom exit function through dependency injection #389

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1360bd7
feat: allow custom exit function through dependency injection
honzajavorek Feb 5, 2025
c5339d9
improve the type annotation
honzajavorek Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions src/apify/_actor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,17 +231,19 @@ async def exit(
event_listeners_timeout: timedelta | None = EVENT_LISTENERS_TIMEOUT,
status_message: str | None = None,
cleanup_timeout: timedelta = timedelta(seconds=30),
sys_exit: Callable = sys.exit,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be passed in when the context manager is used (async with Actor)? Otherwise, I'd be more inclined to just not calling sys.exit from Actor.exit. From my point of view, the proposed solution just lets you do Actor.exit(sys_exit=something) instead of Actor.exit(); something().

) -> None:
"""Exit the Actor instance.

This stops the Actor instance. It cancels all the intervals for regularly sending `PERSIST_STATE` events,
sends a final `PERSIST_STATE` event, waits for all the event listeners to finish, and stops the event manager.

Args:
exit_code: The exit code with which the Actor should fail (defaults to `0`).
exit_code: The exit code with which the program should end (defaults to `0`).
event_listeners_timeout: How long should the Actor wait for Actor event listeners to finish before exiting.
status_message: The final status message that the Actor should display.
cleanup_timeout: How long we should wait for event listeners.
sys_exit: A function to use to terminate the program (defaults to `sys.exit`)
"""
self._raise_if_not_initialized()

Expand All @@ -267,29 +269,31 @@ async def finalize() -> None:
self._is_initialized = False

if is_running_in_ipython():
self.log.debug(f'Not calling sys.exit({exit_code}) because Actor is running in IPython')
self.log.debug(f'Not calling sys_exit({exit_code}) because Actor is running in IPython')
elif os.getenv('PYTEST_CURRENT_TEST', default=False): # noqa: PLW1508
self.log.debug(f'Not calling sys.exit({exit_code}) because Actor is running in an unit test')
self.log.debug(f'Not calling sys_exit({exit_code}) because Actor is running in an unit test')
elif hasattr(asyncio, '_nest_patched'):
self.log.debug(f'Not calling sys.exit({exit_code}) because Actor is running in a nested event loop')
self.log.debug(f'Not calling sys_exit({exit_code}) because Actor is running in a nested event loop')
else:
sys.exit(exit_code)
sys_exit(exit_code)
Comment on lines 271 to +278
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this logic could be simplified a fair bit.

  • we don't need to check if we're running under pytest - we can instead pass in a mock sys_exit
  • didn't @vdusek manage to remove the nesting?
  • we probably still have to check for ipython 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding the nest-asyncio ...

We managed to resolve the problem of executing the Actor & Spider together. Here is more context and the core result structure - scrapy/scrapy#6639 (reply in thread).

However, there's still one remaining challenge - executing asyncio coroutines from the synchronous Scheduler (and HTTP cache storage) code. This is in-progress.

Once that's addressed, I'll open a new PR removing nest-asyncio completely.


async def fail(
self,
*,
exit_code: int = 1,
exception: BaseException | None = None,
status_message: str | None = None,
sys_exit: Callable = sys.exit,
) -> None:
"""Fail the Actor instance.

This performs all the same steps as Actor.exit(), but it additionally sets the exit code to `1` (by default).

Args:
exit_code: The exit code with which the Actor should fail (defaults to `1`).
exit_code: The exit code with which the program should fail (defaults to `1`).
exception: The exception with which the Actor failed.
status_message: The final status message that the Actor should display.
sys_exit: A function to use to terminate the program (defaults to `sys.exit`)
"""
self._raise_if_not_initialized()

Expand All @@ -298,7 +302,7 @@ async def fail(
if exception and not is_running_in_ipython():
self.log.exception('Actor failed with an exception', exc_info=exception)

await self.exit(exit_code=exit_code, status_message=status_message)
await self.exit(exit_code=exit_code, status_message=status_message, sys_exit=sys_exit)

def new_client(
self,
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/actor/test_actor_log.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,4 +91,4 @@ async def test_actor_logs_messages_correctly(
assert caplog.records[10].message == 'Exiting Actor'

assert caplog.records[11].levelno == logging.DEBUG
assert caplog.records[11].message == 'Not calling sys.exit(91) because Actor is running in an unit test'
assert caplog.records[11].message == 'Not calling sys_exit(91) because Actor is running in an unit test'
Loading