Skip to content

async and await for send_log_message in fastmcp#172

Merged
dsp-ant merged 1 commit intomodelcontextprotocol:mainfrom
sheffler:fixAsyncLog
Jan 27, 2025
Merged

async and await for send_log_message in fastmcp#172
dsp-ant merged 1 commit intomodelcontextprotocol:mainfrom
sheffler:fixAsyncLog

Conversation

@sheffler
Copy link
Copy Markdown
Contributor

Motivation and Context

context.send_log_message is async, callers of this method in fastmcp need to be async as well

How Has This Been Tested?

Yes, with Inspector. With the change, error messages about send_log_message "not awaited" have gone away. I do not, however, see the log messages in the Inspector notification pane.

Breaking Changes

Yes, users of Context.info(), etc will need to await.

Types of changes

  • [ x] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • [ x] I have read the MCP Documentation
  • [x ] My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

Copy link
Copy Markdown
Member

@dsp-ant dsp-ant left a comment

Choose a reason for hiding this comment

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

Good catch, thank you

@dsp-ant
Copy link
Copy Markdown
Member

dsp-ant commented Jan 27, 2025

Sorry I had to revert this. It caused tests to fail. feel free to reopen with fixed tests.

@sheffler
Copy link
Copy Markdown
Contributor Author

Will investigate and try to fix tests.

@dsp-ant
Copy link
Copy Markdown
Member

dsp-ant commented Jan 27, 2025

actually i found the error, i fixed it.

@barino-carvana
Copy link
Copy Markdown

Did these changes not make it into the 1.2.1 version?

@regisb
Copy link
Copy Markdown

regisb commented May 7, 2025

Did we forget to update the documentation after this PR was merged? I see several non-async references to ctx.info in the codebase. Would you like me to open a PR?

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.

4 participants