Skip to content

Streamable Http - clean up server memory streams #604

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

Merged
merged 76 commits into from
May 2, 2025

Conversation

ihrpr
Copy link
Contributor

@ihrpr ihrpr commented Apr 29, 2025

Saving only one read stream in _request_streams is not right as we need to clean up both - read and write streams.

@ihrpr ihrpr marked this pull request as ready for review April 29, 2025 19:31
@ihrpr ihrpr added this to the 2025-03-26 spec release milestone Apr 29, 2025
bhosmer-ant
bhosmer-ant previously approved these changes May 1, 2025
Copy link
Contributor

@bhosmer-ant bhosmer-ant left a comment

Choose a reason for hiding this comment

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

LGTM, couple minor its inline but your call whether they're worth the time

await write_stream.aclose()
except Exception as e:
logger.debug(f"Error closing streams: {e}")
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant?

await self._request_streams[request_id][0].aclose()
await self._request_streams[request_id][1].aclose()
except Exception as e:
logger.debug(f"Error closing memory streams: {e}")
Copy link
Contributor

Choose a reason for hiding this comment

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

What exceptions would we expect to see here? I'm slightly worried about swallowing everything, since (I think) aclose() itself shouldn't normally throw anything

await self._write_stream_reader.aclose()
if self._write_stream is not None:
await self._write_stream.aclose()
except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same q about possibly narrowing down the exceptions we really want to suppress here (maybe all? but just flagging)

Base automatically changed from ihrpr/client-resumability to main May 2, 2025 13:49
@ihrpr ihrpr dismissed bhosmer-ant’s stale review May 2, 2025 13:49

The base branch was changed.

@ihrpr ihrpr merged commit 5d8eaf7 into main May 2, 2025
9 checks passed
@ihrpr ihrpr deleted the ihrpr/server-closing-streams branch May 2, 2025 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants