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

[catmem] Release Queue Descriptor Even If close() Fails #691

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

ppenna
Copy link
Contributor

@ppenna ppenna commented Apr 25, 2023

Description

This PR is a follow up of #686

Summary of Changes

  • Changed close() to release the queue descriptor even if we fail to push_eof().

#686

@ppenna ppenna added the enhancement Enhancement Request on an Existing Feature label Apr 25, 2023
@ppenna ppenna self-assigned this Apr 25, 2023
Copy link
Contributor

@iyzhang iyzhang left a comment

Choose a reason for hiding this comment

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

LGTM

src/rust/catmem/mod.rs Outdated Show resolved Hide resolved
@iyzhang
Copy link
Contributor

iyzhang commented Apr 25, 2023

instead of freeing the queue descriptor, should we eventually have states and the queue should be placed into a CLOSING state, which continues to try to push the EoF?

@ppenna
Copy link
Contributor Author

ppenna commented Apr 25, 2023

instead of freeing the queue descriptor, should we eventually have states and the queue should be placed into a CLOSING state, which continues to try to push the EoF?

Definitely we should have some abstraction for the states of the queue itself, such as we have in Catnap Loop. However, the problem here about continuing try to push EoF is that we are not sure that this is due because the ring buffer is full or the other end has closed the pipe. In the latter case, we don't have a signal mechanism, because a pipe is unidirectional. Any ideias?

@iyzhang
Copy link
Contributor

iyzhang commented Apr 26, 2023

instead of freeing the queue descriptor, should we eventually have states and the queue should be placed into a CLOSING state, which continues to try to push the EoF?

Definitely we should have some abstraction for the states of the queue itself, such as we have in Catnap Loop. However, the problem here about continuing try to push EoF is that we are not sure that this is due because the ring buffer is full or the other end has closed the pipe. In the latter case, we don't have a signal mechanism, because a pipe is unidirectional. Any ideias?

My thought was that we should at least keep trying to push EoF regardless of whether the other end will read it because it closed the pipe or stopped pulling things out of it.

@ppenna
Copy link
Contributor Author

ppenna commented Apr 26, 2023

instead of freeing the queue descriptor, should we eventually have states and the queue should be placed into a CLOSING state, which continues to try to push the EoF?

Definitely we should have some abstraction for the states of the queue itself, such as we have in Catnap Loop. However, the problem here about continuing try to push EoF is that we are not sure that this is due because the ring buffer is full or the other end has closed the pipe. In the latter case, we don't have a signal mechanism, because a pipe is unidirectional. Any ideias?

My thought was that we should at least keep trying to push EoF regardless of whether the other end will read it because it closed the pipe or stopped pulling things out of it.

Sure, I've opened an issue for this.

@ppenna ppenna merged commit 88eb290 into dev Apr 26, 2023
10 checks passed
@ppenna ppenna deleted the enhancement-catmem-close branch April 26, 2023 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement Request on an Existing Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants