Skip to content
This repository was archived by the owner on Jun 27, 2024. It is now read-only.

Conversation

@rustatian
Copy link
Member

@rustatian rustatian commented Oct 4, 2023

Reason for This PR

  • Incorrect debug mode streaming information.

Description of Changes

  • Missed the first frame of the stream.
  • Incorrect stop operation.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

Summary by CodeRabbit

Chore: Enhanced Debugging Information

  • Added more detailed logging for debugging in pool/static_pool/debug.go and worker/worker.go. These changes provide better visibility into the worker lifecycle, specifically when a worker is stopped or destroyed.
  • Integrated with "github.com/roadrunner-server/sdk/v4/fsm" to manage worker states effectively. This will help in tracking and controlling the worker's state transitions during debug mode.

Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
@rustatian rustatian added the B-bug Bug: bug, exception label Oct 4, 2023
@rustatian rustatian requested a review from wolfy-j October 4, 2023 10:05
@rustatian rustatian self-assigned this Oct 4, 2023
@coderabbitai
Copy link

coderabbitai bot commented Oct 4, 2023

Walkthrough

The changes primarily focus on enhancing the debug mode functionality in the worker pool. They introduce log messages to indicate when a worker is stopped and when it's running in debug mode, with the latter also triggering worker destruction post-response.

Changes

File Summary
.../static_pool/debug.go Added import for "github.com/roadrunner-server/sdk/v4/fsm". Introduced log message for debug mode execution and worker destruction post-response. Implemented call to w.Stop() to halt worker in debug mode.
worker/worker.go Added a debug log message in the Stop() method of the Process struct, indicating the cessation of the worker along with its PID.

🐇💻

In the realm where code rabbits hop,
Debug logs help us know when to stop.
Workers rest after their response,
Enhancements dance in this merry prance! 🎉🥳


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • Mention @coderabbitai in any review comment for bot assistance.
  • Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

  • Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.
  • Additionally, enter @coderabbitai pause to halt the bot's ongoing reviews as any issue or review comment. To
    resume reviews, enter @coderabbitai resume as any issue or review comment.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Details

Commits Files that changed from the base of the PR and between a2fa730 and 807154d.
Files selected for processing (2)
  • pool/static_pool/debug.go (4 hunks)
  • worker/worker.go (1 hunks)
Files skipped from review due to trivial changes (1)
  • worker/worker.go
Additional comments (Suppressed): 4
pool/static_pool/debug.go (4)
  • 16-20: The new import statement for the package "github.com/roadrunner-server/sdk/v4/fsm" has been added. Ensure that this package is correctly installed and available in your project dependencies.

  • 17-17: A debug log message has been added to indicate when the code is executing in debug mode, and that the worker will be destroyed after receiving a response. This is a good practice as it provides more visibility into the application's behavior during debugging.

  • 64-69: The stop operation has been updated to include a context with timeout. This is a good practice as it prevents potential blocking of the stop operation if the stream cannot be cancelled immediately. However, ensure that the value of sp.cfg.StreamTimeout is appropriately set to prevent premature cancellation.

  • 95-104: The worker is now stopped using w.Stop() instead of being released back to the pool. This change aligns with the debug log message indicating that the worker will be destroyed after a response is received. However, ensure that this change does not lead to resource leaks or other unintended side effects due to workers not being reused.

@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Merging #98 (807154d) into master (a2fa730) will decrease coverage by 0.72%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master      #98      +/-   ##
==========================================
- Coverage   77.82%   77.10%   -0.72%     
==========================================
  Files          23       23              
  Lines        2070     2088      +18     
==========================================
- Hits         1611     1610       -1     
- Misses        405      423      +18     
- Partials       54       55       +1     
Files Coverage Δ
worker/worker.go 67.93% <100.00%> (+0.07%) ⬆️
pool/static_pool/debug.go 23.68% <13.04%> (-5.13%) ⬇️

... and 2 files with indirect coverage changes

@rustatian rustatian merged commit 1c017c8 into master Oct 4, 2023
@rustatian rustatian deleted the fix/debug-streaming-mode branch October 4, 2023 12:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

B-bug Bug: bug, exception

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants