fix:close transport on connection failure#1862
fix:close transport on connection failure#1862sid293 wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
…during client connect
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
felixweinberger
left a comment
There was a problem hiding this comment.
Hi @sid293 thanks for this - did you come across this in something you were working on or is this purely speculative based on reading the source?
Some context for how you ran into this issue would be great.
| headers: { | ||
| 'user-agent': 'MyApp/1.0' | ||
| } | ||
| }); |
There was a problem hiding this comment.
If we need this type assertion to make this work, that's a sign the solution isn't quite there yet.
There was a problem hiding this comment.
-
regarding the
connect()change, I came across while using the library, when connection failed to connect it was not automatically closed and saw some comments saying maybe it should. -
regarding the type assertion, I was getting red underline on my editor for
cache.
and I thought of writing something like this:
declare global {
type RequestCache = "default" | "force-cache" | "no-cache" | "no-store" | "only-if-cached" | "reload";
interface RequestInit {
cache?: RequestCache;
}
}
but looking a bit more I found out it was because my editor's typescript version was not updated and maybe in previous versions cache was not included in RequestInit type.
sorry I will revert back to previous commit
28792b5 to
511c1a1
Compare
Fixes a resource leak on connection failure and resolves TypeScript error in
packages/client/test/client/auth.test.ts.Motivation and Context
client.ts: Movedsuper.connect()inside thetryblock so thecatchblock can properly close the transport if initialization fails.auth.test.ts: Addedas unknown as RequestInitto resolve a type mismatch on the optionalcacheproperty.How Has This Been Tested?
Added a new integration test to verify that the transport is closed even when startup fails.
All tests were verified passing using the workspace-wide command:
pnpm test:allBreaking Changes
none
Types of changes
Checklist
Additional context