-
Notifications
You must be signed in to change notification settings - Fork 284
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
Eliminate unnecessarily scary log lines #4851
Conversation
The module ThreadLocalTable uses a Map data structure to keep track of the name to use for a thread in the logs. A name is normally registered through a call to `Debug.with_thread_associated`, soon after a thread is created. However, each time a new context is created through a function like `Server_helpers.exec_with_new_task`, `Debug.with_thread_associated` is called as well, which then overwrites the name for the current thread. When the inner task exits, the old name is not restore, but simply left blank. The result is the useful information end up missing from the first part of log lines. For example. we see this [error||128 ||xenops] rather than [error||128 |xapi events D:4f8a44d7e8d1|xenops] In the past, there was a Hashtbl instead of a Map here, which does remember all previous bindings in the table. The Hashtbl was removed in order to avoid taking the lock in `find`. This patchs adds a list inside each Map value to model a stack, which behaves like Hashtbl, but in a pure functional way. Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
Draft: still running tests. |
The session.logout was used to interrupt the call to `event.from`, so that is can be called again with a new list of VMs to watch, in case a VM started, stopped etc. The trouble is, that each such re-register event led to SESSION_INVALID being logged, with stack trace, three times. This caused many people looking at the logs unnecessary worries, while in reality the behaviour is completely normal. This replaces the register mechanism by adding the event loop's task into the watch list for `event.from`, and injecting an event on the task to interrupt the call and recompute the VM list. Signed-off-by: Rob Hoes <rob.hoes@citrix.com>
7078d05
to
65b6012
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic to watch event looks quite complex, but I cannot spot any obvious issue with the change.
(previously exceptions were used to interrupt a while loop, not a guard is used)
BVT and BST test suites passed, except for one thing, which @psafont is checking for relevance to this change. |
Test failure looks like something else, so this change is clear. |
shoutout to make @rwdrich feel vindicated :) |
No description provided.