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 logging getting stuck when maxlog is specified and don't print maxlog, fixes #1902 #1911

Merged
merged 3 commits into from
Feb 15, 2022

Conversation

Joel-Dahne
Copy link
Contributor

This is an attempt at fixing #1902. It contains two commits. The first one fixes it so that it doesn't get stuck if multiple logs are encountered when maxlog is used on one of them. The second one removes the maxlog from the log-message, in line with how Julia handles it. It also adds tests for both of these behaviors.

This is the first time I'm contributing to Pluto so there are some things it would be good if they could be check.

  1. At the moment it deletes the maxlog element from the log to not have it be printed in the log-message. I'm not sure if this is the right way to do it. I don't know if updating the log is okay, maybe it is used in other places as well? Maybe it is better to exclude maxlog from the message in the frontend instead of backend?
  2. When writing the tests I mimicked the one existing test. However I don't understand the use of poll so I might have misused it.

The previous version would hang of there were multiple logs and one of
them had maxlog set.
@github-actions
Copy link
Contributor

Try this Pull Request!

Open Julia and type:

julia> import Pkg
julia> Pkg.activate(temp=true)
julia> Pkg.add(url="https://github.com/Joel-Dahne/Pluto.jl", rev="maxlog-stuck")
julia> using Pluto

@fonsp
Copy link
Owner

fonsp commented Feb 12, 2022

I added documentation for poll, hope that helps! a14bcba

@fonsp
Copy link
Owner

fonsp commented Feb 12, 2022

Thanks so much for adding tests, really helpful!!

It now uses poll only in the first step when waiting for all log
message to arrive.
@Joel-Dahne
Copy link
Contributor Author

I added documentation for poll, hope that helps! a14bcba

Armed with this knowledge I slightly rewrote the tests!

@Joel-Dahne
Copy link
Contributor Author

I'm not sure if the failed tests are related?

@fonsp
Copy link
Owner

fonsp commented Feb 15, 2022

Perfect, thanks so much!

@fonsp fonsp merged commit 63748e1 into fonsp:main Feb 15, 2022
@Joel-Dahne Joel-Dahne deleted the maxlog-stuck branch February 15, 2022 08:09
@fonsp fonsp added the logging About `@info`, `@warn`, etc label Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logging About `@info`, `@warn`, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notebooks hangs when maxlog is used in logging and includes maxlog in output Logging doesn't respect maxlog
2 participants