Skip to content

[#63] Fix HTTP transport by removing duplication of base transport #66

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

rvoh-emccaleb
Copy link

@rvoh-emccaleb rvoh-emccaleb commented Jan 27, 2025

Issue

#63

Explanation of bug

The http.HTTPTransport duplicated much of the http.baseTransport's fields and methods.

This resulted in a deadlock caused by a non-nil channel being waited on for a receive and never receiving anything because we have a nil handler. The handler is nil because the protocol is setting the message handler using http.HTTPTransport.SetMessageHandler(), which did not have the same outcome as http.HTTPTransport.baseTransport.SetMessageHandler(), due to the duplicated fields and methods in the two structs.

Solution

Removed the duplication of http.baseTransport fields and methods from the http.HTTPTransport.

I can now start up the server, locally, and observe the following:

% curl -v -X POST http://localhost:8080/mcp \
  -H "Content-Type: application/json" \
  -d '{
    "jsonrpc": "2.0",
    "method": "initialize",
    "id": 1,
    "params": {}
  }'
Note: Unnecessary use of -X or --request, POST is already inferred.
* Host localhost:8080 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:8080...
* Connected to localhost (::1) port 8080
> POST /mcp HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/8.7.1
> Accept: */*
> Content-Type: application/json
> Content-Length: 85
> 
* upload completely sent off: 85 bytes
< HTTP/1.1 200 OK
< Content-Type: application/json
< Date: Mon, 27 Jan 2025 18:44:19 GMT
< Content-Length: 217
< 
* Connection #0 to host localhost left intact
{"id":1,"jsonrpc":"2.0","result":{"capabilities":{"prompts":{"listChanged":false},"resources":{"listChanged":false},"tools":{"listChanged":false}},"protocolVersion":"2024-11-05","serverInfo":{"name":"","version":""}}}% 

Prior to the changes, this would have hung indefinitely (deadlock).

@rvoh-emccaleb
Copy link
Author

Accidentally deleted the branch on my fork -- this PR is still open

@Chrisbattarbee
Copy link
Contributor

Chrisbattarbee commented Jan 28, 2025

Hey @rvoh-emccaleb ! Thanks for the contribution, currently a little strapped for time but hoping to review this tomorrow if that works for you? Same with the other PR

@rvoh-emccaleb
Copy link
Author

@Chrisbattarbee Sounds good!

No rush on timing -- I'm currently running off of my fork. Sorry that these PRs got a little muddied with the histories (the stuff we see above) -- I mismanaged my branches, and now there is a sequential ordering to these PRs. First this one, then the other one.

@rvoh-emccaleb
Copy link
Author

Temporarily have to close this due to company OSS policies

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