Skip to content

Refactor transports#2851

Merged
rchl merged 12 commits intomainfrom
transports-refactor
Apr 9, 2026
Merged

Refactor transports#2851
rchl merged 12 commits intomainfrom
transports-refactor

Conversation

@rchl
Copy link
Copy Markdown
Member

@rchl rchl commented Apr 6, 2026

Refactor transports implementation.

This is based on refactoring in #2015 but with quite notable changes:

  • obviously doesn't include anything related to node-ipc support
  • doesn't include some additions done there that didn't have anything to do with node-ipc - websocket or duplex pipe transport
  • doesn't refactor ClientConfig transport options - everything is controlled with the same settings as before
  • refactors types - generics weren't really useful for transports since we can't infer at runtime within Session or plugin which transport is used. Instead TransportWrapper is just an opaque controller for the transport that Session sees. Everything is now properly typed - previously we had issues with subprocess types and what not.
  • A lot of changes to fix some issues with process going into a 100% cpu loop.

More detailed description of the changes (AI generated):

plugin/core/transports.py (major rewrite)

  • Removed generics — Transport[T], TransportCallbacks[T_contra], AbstractProcessor[T] are gone. Everything now operates directly on JSONRPCMessage (a concrete type alias).
  • TransportConfig moved in — previously lived in types.py; now an abstract base class (ABC) in transports.py, with three concrete subclasses:
    • StdioTransportConfig — stdio-based subprocess transport
    • TcpClientTransportConfig — text editor acts as TCP client
    • TcpServerTransportConfig — text editor acts as TCP server/listener
  • ProcessTransportTransportWrapper** — renamed and simplified; no longer generic, drops the socket/stderr fields, now holds a Transport instance and an ErrorReader.
  • Transport refactored — now a plain class with read(), write(), close() methods, with two concrete subclasses: FileObjectTransport and SocketTransport.
  • ErrorReader extracted — stderr/stdout log reading was previously inline in ProcessTransport; now a separate class.
  • LaunchConfig added — small helper encapsulating command + env + subprocess launch.
  • JsonRpcProcessor removed — encode/decode promoted to module-level encode_json / decode_json functions.
  • create_transport() removed — replaced by the polymorphic TransportConfig.start() dispatch.

plugin/core/types.py

  • TransportConfig and TCP_CONNECT_TIMEOUT removed (moved to transports.py).

plugin/core/sessions.py

  • Session no longer parameterized: TransportCallbacks['dict[str, Any]']TransportCallbacks.
  • self.transport type changed from Transport to TransportWrapper.
  • _response_handlers key type widened from int to str | int (JSON IDs can be strings).
  • deduce_payload / on_payload / response_handler updated to use JSONRPCMessage instead of dict[str, Any].
  • exit() cleaned up: replaced try/except AttributeError hack with a proper if self.transport: guard.

plugin/core/windows.py / plugin/tooling.py

  • Call sites updated to use the new TransportConfig.start() API instead of the old create_transport() function.

tests/test_protocol.py

  • Minor test updates to match the new type signatures.

enabled: bool = True,
initialization_options: DottedDict | None = None,
settings: DottedDict | None = None,
env: dict[str, str | list[str]] | None = None,
Copy link
Copy Markdown
Member Author

@rchl rchl Apr 6, 2026

Choose a reason for hiding this comment

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

Don't quite see why we've allowed a list of strings for the dict value. I don't see this documented in https://lsp.sublimetext.io/client_configuration/ and also I don't see any package using that format so I think it's fine to drop it.

EDIT: After looking at relevant test I see now that it was used to construct strings joined with platform-specific PATH separator but since nothing used that, I still think it's fine to remove.

Copy link
Copy Markdown
Member

@predragnikolic predragnikolic Apr 9, 2026

Choose a reason for hiding this comment

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

Don't quite see why we've allowed a list of strings for the dict value. I don't see this documented in...

These comments are relevant for this #1811 (comment)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Great digging.

Not sure how I feel about it. Whether should remove or keep and document maybe.

@jwortmann
Copy link
Copy Markdown
Member

This is a quite large diff to review, and I'm not really familiar with this code, so I guess if it's just a refactoring and if CI passes feel free to merge.

@rchl
Copy link
Copy Markdown
Member Author

rchl commented Apr 9, 2026

I agree, not easy to review.

Probably the most important part of that code is the old ProcessTransport (new TransportWrapper) which handles reading/writing threads. I've created a diff that just has this change to make it easier to compare - f36a0eb (that branch is not functional - it's just to see the diff better).

@rchl rchl merged commit 991f281 into main Apr 9, 2026
8 checks passed
@rchl rchl deleted the transports-refactor branch April 9, 2026 19:45
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.

3 participants