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.

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

Jump to bottom

[ESI][Runtime] Teardown: memory ownership #7457

Open
teqdruid opened this issue Aug 7, 2024 · 0 comments
Open

[ESI][Runtime] Teardown: memory ownership #7457

teqdruid opened this issue Aug 7, 2024 · 0 comments
Labels
ESI

Comments

@teqdruid
Copy link
Contributor

teqdruid commented Aug 7, 2024

When an AcceleratorConnection is torn down and destructed, accesses to its associated Accelerators and their associated ports will be memory violations. This is (fairly) well documented in the API, but isn't ideal. Accelerator shouldn't be owned by AcceleratorConnection. The associated ports should merely throw exceptions on accesses to disconnected or destructed connections.

The basic idea is that Accelerator and everything accessible from it should merely be a facade.
@teqdruid teqdruid added the ESI label Aug 7, 2024
teqdruid added a commit that referenced this issue Aug 7, 2024
@teqdruid
[ESI][Runtime] Poll method and optional service thread polling
5cbb873 
Add a poll method to ports, a master poll method to the Accelerator, and
the ability to poll from the service thread. Also, only spin up the
service thread if it's requested.

The service thread polling (in particular) required some ownership
changes: Accelerator objects now belong to the AcceleratorConnection so
that the ports aren't destructed before the service thread gets shutdown
(which causes an invalid memory access). This particular binding isn't
ideal, is brittle, and will be an issue for anything doing the polling.
Resolving #7457 should mitigate this issue.

Backends are now _required_ to call `disconnect` in their destructor.
@teqdruid teqdruid mentioned this issue Aug 7, 2024
Merged
@teqdruid teqdruid changed the title [ESI][Runtime] Teardown: ownership [ESI][Runtime] Teardown: memory ownership Aug 7, 2024
teqdruid added a commit that referenced this issue Aug 8, 2024
@teqdruid
[ESI][Runtime] Poll method and optional service thread polling
1cec4b6 
Add a poll method to ports, a master poll method to the Accelerator, and
the ability to poll from the service thread. Also, only spin up the
service thread if it's requested.

The service thread polling (in particular) required some ownership
changes: Accelerator objects now belong to the AcceleratorConnection so
that the ports aren't destructed before the service thread gets shutdown
(which causes an invalid memory access). This particular binding isn't
ideal, is brittle, and will be an issue for anything doing the polling.
Resolving #7457 should mitigate this issue.

Backends are now _required_ to call `disconnect` in their destructor.
teqdruid added a commit that referenced this issue Aug 8, 2024
@teqdruid
[ESI][Runtime] Poll method and optional service thread polling (#7460)
ad91378 
Add a poll method to ports, a master poll method to the Accelerator, and the ability to poll from the service thread. Also, only spin up the service thread if it's requested.

The service thread polling (in particular) required some ownership changes: Accelerator objects now belong to the AcceleratorConnection so that the ports aren't destructed before the service thread gets shutdown (which causes an invalid memory access). This particular binding isn't ideal, is brittle, and will be an issue for anything doing the polling. Resolving #7457 should mitigate this issue.

Backends are now _required_ to call `disconnect` in their destructor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
ESI
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@teqdruid