Skip to content

Conversation

@sebthom
Copy link
Contributor

@sebthom sebthom commented Oct 25, 2025

Wrap provided OutputStream in BufferedOutputStream to amortize small writes; improves throughput by reducing syscalls. Centralize wrapping via setOutput to avoid double wrapping.

Wrap provided OutputStream in BufferedOutputStream to amortize small
writes; improves throughput by reducing syscalls. Centralize wrapping
via setOutput to avoid double wrapping.
@sebthom sebthom force-pushed the StreamMessageConsumer_OutputBuffering branch from 8827c19 to 4386150 Compare October 25, 2025 22:14
Copy link
Contributor

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

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

This PR contains 3 different changes:

  1. synchronizing changes of output on outputlock (I assume so that output can't change in the middle of a message)
  2. fixing test suite by using atomic string
  3. wrapping output in buffered output stream.

1 and 2 are in principle fine - 3 has an API problem, the getOutput method now unexpectedly no longer returns what I passed in to setOutput or the constructor.

I also don't really like the need to lock to change the output - I think the output should be unchangeable. LSP4E certainly doesn't seem to change it. Not sure of others.

I would be inclined for LSP4J 1.0.0 to remove the public setOutput/getOutput and then simplify this code.

@sebthom
Copy link
Contributor Author

sebthom commented Nov 19, 2025

@jonahgraham maybe you could create another 0.x release where the getOutput/setOutput are deprecated and marked for removal.

@jonahgraham
Copy link
Contributor

@jonahgraham maybe you could create another 0.x release where the getOutput/setOutput are deprecated and marked for removal.

Major releases are API breaking. I don't think we need to deprecate it ahead of time. See https://github.com/eclipse-lsp4j/lsp4j/blob/main/README.md#published-api-changes (just merged from #921).

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