k6/browser: Fix race conditions from stringifying types #4794
Conversation
|
Unfortunately making this race seems ... impossible within a test :( Even the case I have seen takes minutes of fully running load test run with 500 browsers to happen. |
Before this in cases like #4564 you can have an error that stringifies ElementHandle object. That object has many other objects inside of it, some of which have maps or other more complex structure. None of which are thread safe to be accessed without locking. This is very likely the reason for the race conditions in #4564 (maybe other cases as well). In this case we do have ElementHandle as the argument - we get an error due to likely it getting disposed while converting it. And then we start returning an error that has it just stringified. While fmt goes through the object we get another event (maybe exactly the same) modifying one of the many maps within the object - likely under the Frame specifically. This also likely leads to pretty useless logs due to the amount of fields both have. With this change we only logs some fields that are less likely to be racy, or at least won't lead to a runtime race condition without the race detector being on. Fixes #4564
| // String returns a string repesentation of ElementHandle. | ||
| // It exists mostly for debugging where we don't want fmt.Sprintf to just | ||
| // go through a complex object and try to stringify it. | ||
| func (h *ElementHandle) String() string { |
There was a problem hiding this comment.
Are these string methods called when a user works with console.log in their test scripts?
If so, I'm wondering if we should try to expose more of the entities/types. Maybe we need to take a look at what is printed when working with Playwright while performing the same console.log call against the ElementHandle -- happy to help if needed.
There was a problem hiding this comment.
I think that due to the mapping files this is nto being called. But I guess that might an interesting thing to check - yes.
But this currently is about when we create errors with fmt.Errorf and friends and that traversing objects that ... likely shouldn't be traversed with some amount of locking and that leading to races.
Your suggestion is about UX whcih is also nice - but I do not know what the expected result is and it likely is bigger topic.
There was a problem hiding this comment.
Aha, thanks for clarifying 👍
What?
Before this in cases like #4564 you can have an error that stringifies ElementHandle object. That object has many other objects inside of it, some of which have maps or other more complex structure. None of which are thread safe to be accessed without locking.
This is very likely the reason for the race conditions in #4564 (maybe other cases as well). In this case we do have ElementHandle as the argument - we get an error due to likely it getting disposed while converting it. And then we start returning an error that has it just stringified. While fmt goes through the object we get another event (maybe exactly the same) modifying one of the many maps within the object - likely under the Frame specifically.
This also likely leads to pretty useless logs due to the amount of fields both have. With this change we only logs some fields that are less likely to be racy, or at least won't lead to a runtime race condition without the race detector being on.
Why?
Fixing race conditions
Checklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
Fixes #4564