Skip to content

Remove NowhereBufferedSink#14668

Closed
JesseWeinstein wants to merge 2 commits into
signalapp:mainfrom
JesseWeinstein:remove_NowhereBufferedSink
Closed

Remove NowhereBufferedSink#14668
JesseWeinstein wants to merge 2 commits into
signalapp:mainfrom
JesseWeinstein:remove_NowhereBufferedSink

Conversation

@JesseWeinstein

@JesseWeinstein JesseWeinstein commented Mar 16, 2026

Copy link
Copy Markdown
Contributor

Contributor checklist

  • I am following the Code Style Guidelines
  • I have tested my contribution on these devices
  • My contribution is fully baked and ready to be merged as is
  • N/A I ensured that all the open issues my contribution fixes are mentioned in the commit message

Description

The NowhereBufferedSink class violates the closed hierarchy guideline, because BufferedSink is a sealed interface. Technically implementing a sealed interface in a different package is allowed (unlike inheriting from a sealed class), but it's a bad idea.

It is replaced by inlining the no-op OutputStream directly in DigestingRequestBody, accessed via a new method, writeToNowhere.

I still need to get my test environment up and running in order to test this -- but it seems safe...

@jeffrey-signal jeffrey-signal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I still need to get my test environment up and running in order to test this -- but it seems safe...

Has this been tested?

It violates the closed hierarchy guideline, because BufferedSink is a sealed interface. Technically implementing a sealed interface in a different package is allowed (unlike inheriting from a sealed class), but it's a bad idea.

It is replaced by inlining the no-op OutputStream directly in DigestingRequestBody, accessed via a new method, `writeToNowhere`.
@JesseWeinstein JesseWeinstein force-pushed the remove_NowhereBufferedSink branch from ff93ba7 to 67ed716 Compare April 15, 2026 15:18
@JesseWeinstein

Copy link
Copy Markdown
Contributor Author

I still need to get my test environment up and running in order to test this -- but it seems safe...

Has this been tested?

Looking into it further, I'm not sure how best to test this! It looks like it is used for uploading attachments and backup files, and only when it tries to resume an already uploaded file. Suggestions are welcome.

@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions!

@github-actions

Copy link
Copy Markdown

This pull request has been closed due to inactivity.

@github-actions github-actions Bot closed this Jun 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants