-
Notifications
You must be signed in to change notification settings - Fork 65
Feat/add streamable http #126
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
Feat/add streamable http #126
Conversation
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
684fb67
to
4c485fb
Compare
4c485fb
to
3d00d39
Compare
client/call.go
Outdated
@@ -220,7 +220,7 @@ func (client *Client) sendNotification4Initialized(ctx context.Context) error { | |||
// Responsible for request and response assembly | |||
func (client *Client) callServer(ctx context.Context, method protocol.Method, params protocol.ClientRequest) (json.RawMessage, error) { | |||
if !client.ready.Load() && (method != protocol.Initialize && method != protocol.Ping) { | |||
return nil, fmt.Errorf("client not ready") | |||
return nil, fmt.Errorf("callServer: client not ready") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
直接 errors.New 好一些
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
|
||
resp, err := t.client.Do(req) | ||
if err != nil { | ||
t.logger.Errorf("failed to connect to SSE stream: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个地方是不是少了 resp.Body.Close()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个地方不调用close,在下面会close,这里主要是在for循环里,不好使用defer
t.sseInFlyConnect.Add(1) | ||
defer t.sseInFlyConnect.Done() | ||
|
||
t.handleSSEStream(resp.Body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这边是协程,resp.Body是不是在外面关掉了
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
if !server.sessionManager.IsExistSession(sessionID) { | ||
return pkg.ErrLackSession | ||
func (server *Server) receive(ctx context.Context, sessionID string, msg []byte) (<-chan []byte, error) { | ||
if sessionID != "" && !server.sessionManager.IsActiveSession(sessionID) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsActiveSession 和 IsClosedSession 可以合成一个 IsSessionAvailable,然后在里面返回错误
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里合并后IsSessionAvailable返回err,语义上有点奇怪,这里留下IsActiveSession函数,后面也可能会有用吧。
server/session/manager.go
Outdated
"github.com/ThinkInAIXYZ/go-mcp/pkg" | ||
) | ||
|
||
type Manager struct { | ||
sessions pkg.SyncMap[*State] | ||
activeSessions pkg.SyncMap[*State] | ||
closedSessions pkg.SyncMap[*State] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个closedSessions里面的数据什么时候清空?还是说直接改成简单value为struct,记录就行。如果要保存这个session的话,是不是取出来之后看State的closed状态也行。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
嗯这里可以把value改为struct
return has | ||
} | ||
|
||
func (m *Manager) GetSession(sessionID string) (*State, bool) { | ||
state, has := m.sessions.Load(sessionID) | ||
state, has := m.activeSessions.Load(sessionID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个前面可以直接加一个 sessionID 为空直接返回 false,不用Load
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Description
Please provide a brief description of the changes in this pull request.
Related Issue
Fixes #(issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
Checklist: