Skip to content

Remove NowhereBufferedSink#14668

Open
JesseWeinstein wants to merge 2 commits intosignalapp:mainfrom
JesseWeinstein:remove_NowhereBufferedSink
Open

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

Conversation

@JesseWeinstein
Copy link
Copy Markdown
Contributor

@JesseWeinstein JesseWeinstein commented Mar 16, 2026

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...

Copy link
Copy Markdown
Contributor

@jeffrey-signal jeffrey-signal left a comment

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.

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