Skip to content
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

Fix HTTP transport message handling #76

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pavelanni
Copy link

I started testing the HTTP transport using the provided server and client in examples/http_example. It didn't work for me, and I asked Cursor/Sonnet to help me with that. After adding logging to different parts of the code, we (of course, mostly Claude/Sonnet, not me) finally found the issue and fixed it. Now, the client works as expected and displays time in different formats. Here is the explanation and the changes:

Issue

The HTTP transport implementation had several issues that prevented proper message handling:

  1. Message handlers were not being properly set on the base transport
  2. Response channels were managed in two different locations (HTTPTransport and baseTransport)
  3. Redundant state was maintained between the transport layers

Changes

  1. Fixed SetMessageHandler to properly set the handler on the base transport while maintaining backward compatibility
  2. Removed redundant responseMap field from HTTPTransport struct
  3. Removed redundant responseMap initialization from NewHTTPTransport
  4. Updated Send method to use the correct responseMap from base transport

Impact

  • HTTP transport now correctly handles JSON-RPC messages and responses
  • Simplified code by removing redundant state
  • Fixed message handler delegation between transport layers

Testing

The changes have been tested with JSON-RPC initialize requests and confirm that:

  • Message handlers are properly called
  • Responses are correctly sent back to clients
  • No messages are lost between transport layers

@rvoh-emccaleb
Copy link

Possible duplicate of #66, but that one is temporarily closed

@pavelanni
Copy link
Author

Possible duplicate of #66, but that one is temporarily closed

Yes, it looks like it's the same issue. Is there a plan to merge the fix (either this one or #66)?

@rvoh-emccaleb
Copy link

@pavelanni -- I heard from @Chrisbattarbee, here, three weeks ago when I first opened #66. But I haven't heard since.

Sounded like he was crunched for time, then. Been running off my fork since.

@a67793581
Copy link

Yes, it looks like it's the same issue. Is there a plan to merge the fix (either this one or #77)?

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