-
Couldn't load subscription status.
- Fork 9
fix(bake): finish waiting for all printers #402
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
Conversation
eb9d58b to
f506e90
Compare
3dbc622 to
f6d4cb6
Compare
| for range projectIDs { | ||
| fmt.Fprintf(os.Stderr, "**waiting for one printer Wait***\n") | ||
| _ = printer.Wait() |
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 loop is calling printer.Wait() once for each project ID, but this doesn't match the actual number of printers that were added. This could lead to incorrect behavior since Wait() is designed to be called exactly once per added printer.
Instead of iterating over projectIDs, consider:
- Tracking the number of printers added during execution and using that count for the loop, or
- Modifying the
SharedPrinterimplementation to handle multipleWait()calls correctly
The current approach might cause issues with synchronization or resource cleanup if the number of project IDs doesn't match the number of printers that were actually added.
| for range projectIDs { | |
| fmt.Fprintf(os.Stderr, "**waiting for one printer Wait***\n") | |
| _ = printer.Wait() | |
| for i := 0; i < printer.Count(); i++ { | |
| _ = printer.Wait() |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Previously, we were only waiting for a single printer to complete. In multi-project builds we need to wait for all to finish. Co-authored-by: Iris Scholten <ischolten.is@gmail.com> Signed-off-by: Chris Goller <goller@gmail.com>
f6d4cb6 to
7b2694f
Compare
| for range projectIDs { | ||
| _ = printer.Wait() | ||
| } |
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.
This change may cause a panic with 'negative WaitGroup counter' since printer.Wait() calls w.wg.Done() internally. Calling it multiple times (once per project ID) will decrement the counter beyond its initial value.
The solution should ensure that the number of Wait() calls matches the number of times the printer was added to the WaitGroup. Consider either:
- Tracking how many times the printer was added and calling
Wait()exactly that many times, or - Modifying the
SharedPrinter.Wait()implementation to safely handle multiple calls
The current implementation of SharedPrinter.Wait() assumes it will be called exactly once per addition.
GitHub Flavored Markdown: No
| for range projectIDs { | |
| _ = printer.Wait() | |
| } | |
| // Only need to wait once for the printer to finish | |
| _ = printer.Wait() |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Previously, we were only waiting for a single printer to complete. In multi-project builds we need to wait for all to finish.