8000
Skip to content

Fix #3167#5332

Open
camoy wants to merge 1 commit intoracket:masterfrom
camoy:log-interceptor
Open

Fix #3167#5332
camoy wants to merge 1 commit intoracket:masterfrom
camoy:log-interceptor

Conversation

@camoy
Copy link
Copy Markdown
Contributor
@camoy camoy commented Sep 12, 2025

Checklist

  • Bugfix
  • tests included

Description of change

This PR fixes the behavior described in #3167 to match both intuitive expectations and the documentation. I did not end up going with the solution that I originally proposed in the issue because it turns out that the strategy the code currently uses is fundamentally broken. If proc captures a continuation and restores it later, it can cause channel-put to execute after the thread has terminated, resulting in hanging. The solution I ended up going with, which resolves both this issue and my original one, is to use dynamic-wind to launch and teardown the receiver thread when execution enters and exits the dynamic extent of proc.

I have two tests. The first confirms that #3167 is fixed. The second confirms that logs are intercepted when a continuation is invoked that re-enters proc.

@mflatt
Copy link
Copy Markdown
Member
mflatt commented Sep 12, 2025

What effect does this change have on existing packages that use with-intercepted-logging with #:logger?

Some packages that seem to use it (not a complete list): video, pollen, resyntax (in tests), make-log-interceptor, and trivial.

@camoy
Copy link
Copy Markdown
Contributor Author
camoy commented Sep 12, 2025

That's a good question, I just took a quick look at these packages.

video

Used in two places. The first doesn't use the logs at all and seems to just be a workaround. The second is interesting in that it seems to me that without this PR the code would actually be buggy. There is an assumption that the log message matches one of three regular expressions, but that is only true if the log interceptor only intercepts logs from ffmpeg-logger, which it may not in the original code. After this PR, I think the code is correct.

pollen

Looks like pollen just vendors the old version of with-intercepted-logging for compatibility with old versions of Racket (and consequently doesn't use it).

make-log-interceptor

Wraps with-intercepted-logging with a slightly different interface and functionality. Should not be negatively affected.

trivial

The use here is only in tests and manually filters out logs not related to ttt. So the check on line 67 is redundant with this PR.

@rfindler
Copy link
Copy Markdown
Member

One (very) concrete thing to check is if the tests continue to pass with the change. If they don't, we should take some care because it can be difficult to get some maintainers to accept pull requests.

@camoy
Copy link
Copy Markdown
Contributor Author
camoy commented Sep 13, 2025

I checked and tests pass on these projects with the patch, other than video. I wasn't able to get video to work at all because it requires an older ffmpeg than I have on my machine.

@mfelleisen
Copy link
Copy Markdown
Collaborator
mfelleisen commented Sep 13, 2025 via email

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

0