Skip to content

console: bound queue to 10 items #6486

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.

Sign up for GitHub

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 1 commit into
base: master
Choose a base branch
from
Open

console: bound queue to 10 items #6486

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/core/opamConsole.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,11 @@ let () =

let pending = Queue.create ()

let add_to_queue data =
Queue.push data pending;
if Queue.length pending > 10 then
ignore (Queue.pop pending)

let clear_pending debug_level =
let f (level, timestamp, msg) =
if level <= abs debug_level then
Expand All @@ -565,7 +570,7 @@ let log section ?(level=1) fmt =
if not OpamCoreConfig.(!r.set) then
let b = Buffer.create 128 in
let timestamp = timestamp () ^ " " in
Comment on lines 571 to 572
Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead of removing items from the queue without telling anyone, we could just add one when it reaches the threshold that says that the rest hasn't been added. Though it should be a programing error on our part if this ever happens (be it a threshold value too low or something that we haven't thought of happening), so maybe just showing a warning unconditionally instead could be a solution?

Suggested change
let b = Buffer.create 128 in
let timestamp = timestamp () ^ " " in
let timestamp () = timestamp () ^ " " in
if Queue.length pending = 10 then
Queue.push (1, timestamp (), "truncated logs [...]")
else if Queue.length pending < 10 then
let b = Buffer.create 128 in

Copy link
Member Author

Choose a reason for hiding this comment

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

I've no idea what the purpose of the commit that introduced this queue was, and what the desired semantics are: should the earliest messages be preserved? the latest ones? on which platform(s) does it matter?

depending on the answer to the questions above, I see multiple options: remove the queue entirely; only add the earliest N messages (adding a truncated message is definitively nice); or add the latest N messages (here as well a truncated message would be nice).

Maybe @dra27 who wrote the code years ago can explain his needs when he wrote that code.

Copy link
Member

Choose a reason for hiding this comment

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

actually thinking about it more, i prefer the solution with the warning. This way you would have seen the need to use OpamCoreConfig.init almost immediately when using the library.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean a warning once when the queue is full (and messages are dropped)? Or a warning for each log message being dropped?

Honestly, from the API I read nowhere that OpamCoreConfig.set/update needs to be called at any time -- and there are sane defaults :) -- so I don't quite understand why there should be a warning, and more fundamentally I don't understand that there's a queue...

Copy link
Member

Choose a reason for hiding this comment

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

I've no idea what the purpose of the commit that introduced this queue was, and what the desired semantics are:

The purpose is that --cli needs to be evaluated before OpamCoreConfig because some of the environment variable might not be available with some of the values of CLI for example. So a couple of things happens before OpamCoreConfig.init gets run and log gets called. So since the config hasn't been initialized opam doesn't know yet if something has to be shown on screen so those log messages need to be kept until OpamCoreConfig.init is called.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your excellent explanation.

let k _ = Queue.push (level, timestamp, Buffer.contents b) pending in
let k _ = add_to_queue (level, timestamp, Buffer.contents b) in
Format.kfprintf k (Format.formatter_of_buffer b) ("%a " ^^ fmt ^^ "\n%!")
(acolor_w 30 `yellow) section
else if level <= abs debug_level then
Expand Down
Loading