Skip to content

Fix --log-file option so it doesn't crash the server. #207

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Jun 27, 2025

Description

This fixes #181.

The problem was that calling flush causes the stream to bind to the flush call, so the next time add (or flush) is called, it throws. However the Error was being eaten by a catch somewhere, so I had to dig a bit to get the trace:

Error in writing to logging: Bad state: StreamSink is bound to a stream
# 0      _StreamSinkImpl._controller (dart:io/io_sink.dart:235:7)
# 1      _StreamSinkImpl.add (dart:io/io_sink.dart:155:5)
# 2      DelegatingStreamSink.add (package:async/src/delegate/stream_sink.dart:35:11)
# 3      _createLogSink.<anonymous closure> (package:dart_mcp_server/src/server.dart:293:21)
# 4      _HandlerSink.add (package:async/src/stream_sink_transformer/handler_transformer.dart:61:17)
# 5      MCPBase._maybeForwardMessages.<anonymous closure> (package:dart_mcp/src/shared.dart:196:31)
# 6      _HandlerEventSink.add (dart:async/stream_transformers.dart:230:17)
# 7      _SinkTransformerStreamSubscription._handleData (dart:async/stream_transformers.dart:115:24)
# 8      _rootRunUnary (dart:async/zone.dart:1538:47)
# 9      _CustomZone.runUnary (dart:async/zone.dart:1429:19)
# 10     _CustomZone.runUnaryGuarded (dart:async/zone.dart:1329:7)
...

So, the solution is the same as the old comedy bit: "Patient: Doctor, it hurts when I do this... Doctor: well don't do that".

I just took out the flush and things work, and the the log appears to be flushed at each line anyhow.

It seems that flush in Dart is basically just waiting for the output stream to be empty; it doesn't force anything to be sent to the disk, and since we don't have to make sure things are in the log before adding more to the log, we don't really need the flush.

In the course of investigating, I also found that a there were a couple of uninitialized late final accesses that were also being swallowed in the analyzer support, so I converted them to nullables and only call them if they are set. This was happening when the shutdown occurred on a client that didn't support roots.

Also added a check for an unset protocolVersion because I ran into that too, but I'm not clear on whether it should throw there or just return null. I opted to return null in that case, since a null protocolVersion is allowed.

Related Issues

Tests

  • I un-skipped the related test.

Copy link

github-actions bot commented Jun 27, 2025

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Passing --log-file makes the server stop functioning
1 participant