Skip to content

Capture several server thread exceptions #69

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
kostyanf14 wants to merge 1 commit into
base: master
Choose a base branch
from
+23 −10
Open

Capture several server thread exceptions #69

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
33 changes: 23 additions & 10 deletions lib/server.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,21 +59,34 @@ def deploy_script_file
logger('debug', 'server/initialize') { 'deployed' }
end

def start_server_with_log_fetcher
Copy link

@gemini-code-assist gemini-code-assist bot Feb 6, 2026

Choose a reason for hiding this comment

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

medium

This method appears to be a helper for load_toolshck_server and is not used outside of this class. It's good practice to make such methods private to improve encapsulation. You can use private def to make this specific method private.

  private def start_server_with_log_fetcher

Copy link
Contributor Author

@kostyanf14 kostyanf14 Feb 6, 2026

Choose a reason for hiding this comment

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

it already in the private part

log_l_path = "#{@outp_dir}/#{Time.now.strftime('%d-%m-%Y_%H_%M_%S')}_toolsHCK.log"
File.open(log_l_path, 'a') do |file|
@winrm_ps.send_pipeline_command(process_script) do |message|
if message.parsed_data.respond_to?(:output)
file.print message.parsed_data.output
else
file.print message.parsed_data.raw
end
end
end
end

def load_toolshck_server
logger('debug', 'server/initialize') do
"loading server to listen on port #{@server_port}"
end
@log_fetcher = Thread.new do
log_l_path = "#{@outp_dir}/#{Time.now.strftime('%d-%m-%Y_%H_%M_%S')}_toolsHCK.log"
File.open(log_l_path, 'a') do |file|
@winrm_ps.send_pipeline_command(process_script) do |message|
if message.parsed_data.respond_to?(:output)
file.print message.parsed_data.output
else
file.print message.parsed_data.raw
end
end
end
start_server_with_log_fetcher
rescue WinRM::WinRMError, Errno::ECONNRESET, Errno::ECONNREFUSED, Errno::EPIPE => e
# Safe to ignore as the most likely cause is that the pipeline was closed
# before we finished fetching all messages and we don't want to crash the server
# in that case. This can happen when the server is stopped while we're still fetching messages.
# WinRMError be raised instead of Errno::EPIPE when the WinRM connection is closed while we're
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

Grammar in this comment is off: "WinRMError be raised" should be updated (e.g., "WinRMError can be raised") for clarity.

Suggested change
# WinRMError be raised instead of Errno::EPIPE when the WinRM connection is closed while we're
# WinRMError can be raised instead of Errno::EPIPE when the WinRM connection is closed while we're

Copilot uses AI. Check for mistakes.
# still fetching messages, so we need to catch both.
# In any case, Ether will restart the server before the next command execution,
# so we won't be left with a hanging thread
logger('error', 'server/log_fetcher') { "failed to fetch log message: #{e.message}" }
Copy link

@gemini-code-assist gemini-code-assist bot Feb 6, 2026

Choose a reason for hiding this comment

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

medium

For better debugging, it would be helpful to include the exception class in the log message. This is also consistent with how exceptions are logged elsewhere in the codebase, for example in the log_exception method in ether.rb.

      logger('error', 'server/log_fetcher') { "failed to fetch log message: [#{e.class}] #{e.message}" }

Copy link
Contributor Author

@kostyanf14 kostyanf14 Feb 6, 2026

Choose a reason for hiding this comment

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

We don't need to debug this actually, just don't crash the full application.
@YanVugenfirer Maybe report as a warning instead?

Copy link
Contributor

@YanVugenfirer YanVugenfirer Feb 8, 2026

Choose a reason for hiding this comment

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

Agree

Comment on lines +88 to +89
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

This rescue path is described as "safe to ignore", but it currently logs at error level. That can create noisy/false-positive error logs during normal shutdown/restart. Consider lowering this to warn/debug, and/or including e.class and only using error for truly unexpected failures.

Copilot uses AI. Check for mistakes.
end
logger('debug', 'server/initialize') { 'loaded' }
end
Expand Down
Loading