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

Allow LLEventPump listener access to its own LLBoundListener. #2068

Merged
merged 2 commits into from
Jul 18, 2024

Conversation

nat-goodspeed
Copy link
Collaborator

listen() still takes LLEventListener, a callable(const LLSD&), but now
also accepts LLAwareListener, a callable(const LLBoundListener&, const LLSD&).
This uses boost::signals2::signal::connect_extended(), which, when the
signal is called, passes to a connected listener the LLBoundListener (aka
boost::signals2::connection) representing its own connection. This allows a
listener to disconnect itself when done.

listen() also now accepts LLVoidListener, and internally wraps it in a lambda
that returns false on its behalf.

The tradeoff is that the (unused) LLEventTrackable feature is no longer supported.
That claimed support was always a little bit magical/fragile. IF:

  • a class included LLEventTrackable as a base class AND
  • an instance of that class was managed by boost::shared_ptr AND
  • you passed one of that class's methods and the boost::shared_ptr
    specifically to boost::bind() AND
  • the resulting boost::bind() object was passed into LLEventPump::listen()

THEN the promise was that on destruction of that object, that listener would
automatically be disconnected -- instead of leaving a dangling pointer bound
into the LLEventPump, causing a crash on the next LLEventPump::post() call.

Unfortunately, failing to meet the above conditions meant a silent failure, resulting
in the aforementioned crash.

The only existing code in the viewer code base that exercised LLEventTrackable
functionality was in test programs. When the viewer calls LLEventPump::listen(),
it typically stores the resulting connection object in an LLTempBoundListener
variable, which more reliably guarantees disconnection on destruction of that variable.

The fact that LLEventTrackable support is specific to boost::bind(), that it
silently fails to keep its promise with std::bind() or a lambda or any other
form of C++ callable, makes it untrustworthy for new code.

Note that the code base still uses boost::signals2::trackable for other
boost::signals2::signal instances not associated with LLEventPump. We are
not changing those at this time.

`listen()` still takes `LLEventListener`, a `callable(const LLSD&)`, but now
also accepts `LLAwareListener`, a `callable(const LLBoundListener&, const LLSD&)`.
This uses `boost::signals2::signal::connect_extended()`, which, when the
signal is called, passes to a connected listener the `LLBoundListener` (aka
`boost::signals2::connection`) representing its own connection. This allows a
listener to disconnect itself when done.

Internally, `listen_impl()` now always uses `connect_extended()`. When passed
a classic `LLEventListener`, `listen()` wraps it in a lambda that ignores the
passed `LLBoundListener`.

`listen()` also now accepts `LLVoidListener`, and internally wraps it in a lambda
that returns `false` on its behalf.
Remove documented `LLEventPump` support for `LLEventTrackable`. That claimed
support was always a little bit magical/fragile. IF:

* a class included `LLEventTrackable` as a base class AND
* an instance of that class was managed by `boost::shared_ptr` AND
* you passed one of that class's methods and the `boost::shared_ptr`
  specifically to `boost::bind()` AND
* the resulting `boost::bind()` object was passed into `LLEventPump::listen()`

THEN the promise was that on destruction of that object, that listener would
automatically be disconnected -- instead of leaving a dangling pointer bound
into the `LLEventPump`, causing a crash on the next `LLEventPump::post()` call.

The only existing code in the viewer code base that exercised `LLEventTrackable`
functionality was in test programs. When the viewer calls `LLEventPump::listen()`,
it typically stores the resulting connection object in an `LLTempBoundListener`
variable, which guarantees disconnection on destruction of that variable.

The fact that `LLEventTrackable` support is specific to `boost::bind()`, that it
silently fails to keep its promise with `std::bind()` or a lambda or any other
form of C++ callable, makes it untrustworthy for new code.

Note that the code base still uses `boost::signals2::trackable` for other
`boost::signals2::signal` instances not associated with `LLEventPump`. We are
not changing those at this time.
@nat-goodspeed
Copy link
Collaborator Author

This change will allow a lambda listener to decide for itself when it can stop listening, which is important for the Lua login API.

@nat-goodspeed nat-goodspeed marked this pull request as ready for review July 18, 2024 18:19
/// Support a listener accepting (const LLBoundListener&, const LLSD&).
/// Note that LLBoundListener::disconnect() is a const method: this feature is
/// specifically intended to allow a listener to disconnect itself when done.
typedef LLStandardSignal::extended_slot_type LLAwareListener;
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like name 'aware' might be misleading for a self terminating listener. May be LLTemporaryListener?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mmm, Temporary sounds to me too much as though we're somehow constraining its lifespan.

What I want to convey with the name is the extended signature. LLEnrichedListener? LLExtendedListener?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

LLSelfReferentialListener? LLReflexiveListener? (not really better than the above I think)

Copy link
Contributor

Choose a reason for hiding this comment

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

LLAwareListener is fine enough for me.

Copy link
Contributor

@akleshchev akleshchev Jul 19, 2024

Choose a reason for hiding this comment

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

LLExtendedListener would at least match boost's naming, but comment there is good enough so I guess name wonn't matter as much.

@nat-goodspeed nat-goodspeed merged commit b5c5881 into release/luau-scripting Jul 18, 2024
12 checks passed
@nat-goodspeed nat-goodspeed deleted the lua-pump-notrack branch July 18, 2024 20:24
@github-actions github-actions bot locked and limited conversation to collaborators Jul 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants