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

fix: allow calling Actor.reboot() from migrating handler, align reboot behavior with JS SDK #361

Merged
merged 4 commits into from
Dec 20, 2024

Conversation

fnesveda
Copy link
Member

This fixes several issues with the Actor.reboot() behavior:

  • Actor.reboot() waits for all event handlers to finish, but if itself it was called in an event handler, it would be waiting for itself, getting into a deadlock
  • Actor.reboot() in the JS SDK triggers event handlers for the migrating and persistState events, but in the Python SDK it was triggering only the persistState handlers

This aligns the behavior to work like the JS SDK, and prevents reboot getting into an infinite loop by allowing it to be called only once.

Related PR in JS SDK: apify/apify-sdk-js#345

@fnesveda fnesveda added t-platform Issues with this label are in the ownership of the platform team. adhoc Ad-hoc unplanned task added during the sprint. labels Dec 18, 2024
@fnesveda fnesveda added this to the 105th sprint - Platform Team milestone Dec 18, 2024
@fnesveda fnesveda requested review from janbuchar and vdusek December 18, 2024 14:25
@fnesveda fnesveda self-assigned this Dec 18, 2024
Comment on lines 856 to 861
persist_state_listeners = chain.from_iterable(
(self._event_manager._listeners_to_wrappers[Event.PERSIST_STATE] or {}).values() # noqa: SLF001
)
migrating_listeners = chain.from_iterable(
(self._event_manager._listeners_to_wrappers[Event.MIGRATING] or {}).values() # noqa: SLF001
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something, but the chain.from_iterable doesn't seem to have a purpose here.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does, _listeners_to_wrappers is a dict of dicts of lists of wrappers (because technically you can have one listener wrapped multiple times, even though it doesn't make practical sense).

Imagine this:

def my_event_listener(event_data):
    print(event_data)

Actor.on(Event.MIGRATING, my_event_listener)
Actor.on(Event.MIGRATING, my_event_listener)

Then _listeners_to_wrappers looks like this:

{
    Event.MIGRATING: {
        event_listener: [
            wrapper_for_my_event_listener_1,
            wrapper_for_my_event_listener_2,
        ],
    }
}

So _listeners_to_wrappers[Event.PERSIST_STATE]).values() is a list of lists, and with chain.from_iterable() I flatten it to a single-level list.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, thanks. SDK already depends on Crawlee and that depends on more_itertools which contain flatten - could you add that dependency to SDK as well and use that instead? I know it serves no practical purpose other than readability.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do.

src/apify/_actor.py Show resolved Hide resolved
@fnesveda fnesveda requested a review from janbuchar December 19, 2024 15:46
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.

LGTM

@fnesveda fnesveda merged commit 7ba0221 into master Dec 20, 2024
27 checks passed
@fnesveda fnesveda deleted the fix/prevent-multiple-reboots branch December 20, 2024 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adhoc Ad-hoc unplanned task added during the sprint. t-platform Issues with this label are in the ownership of the platform team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants