-
-
Notifications
You must be signed in to change notification settings - Fork 41
Preserve caller-owned invite target streams #758
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
Conversation
Co-authored-by: Lulalaby <[email protected]>
Co-authored-by: Lulalaby <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73f8051043
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (targetUsersCsv is not null && targetUsersCsv.ResetPositionTo is null) | ||
| { | ||
| // ignore | ||
| try | ||
| { | ||
| targetUsersCsv.Stream.Dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid disposing user-owned non-seekable streams
This disposal guard treats ResetPositionTo == null as “internally owned,” but user-supplied streams that are not seekable also produce ResetPositionTo == null in CreateTargetUsersFile. In that case, the caller’s stream will still be disposed here, which contradicts the intent of preserving caller ownership and can still trigger ObjectDisposedException if the caller reuses the stream after CreateChannelInviteAsync. Consider tracking ownership separately (e.g., a flag from the targetUsersFile parameter) rather than relying on ResetPositionTo alone.
Useful? React with 👍 / 👎.
| else if (targetUsersCsv is not null) | ||
| { | ||
| // ignore | ||
| try | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid disposing user-owned non-seekable streams
This disposal path still closes user-owned streams that are not seekable because CreateTargetUsersFile sets ResetPositionTo only when stream.CanSeek is true. For non-seekable caller streams, ResetPositionTo remains null and this block disposes them anyway, defeating the ownership fix and risking ObjectDisposedException for callers who reuse the stream after UpdateInviteTargetUsersAsync completes.
Useful? React with 👍 / 👎.
Description
Avoid disposing user-supplied target user CSV streams in invite APIs, preventing
ObjectDisposedExceptionafter call completion. Disposal now only occurs for internally-created streams.ResetPositionTo == null, respecting caller-owned streams inCreateChannelInviteAsyncandUpdateInviteTargetUsersAsync.NullReferenceException.Example:
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist:
@Aiko-IT-Systems/discatsharp
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.