isolate eval call so it can't muck with local variables #17
Conversation
nerdrew
commented
Apr 24, 2025
nerdrew
commented
- update rubocop, and fix all rubocop failures
- Refactor specs
nerdrew
force-pushed
the
lazarus/isolated-eval
branch
4 times, most recently
from
0cc67f5 to
7b90edb
Compare
doctown
left a comment
doctown
left a comment
There was a problem hiding this comment.
Thanks for working on this.
| socket&.close | ||
| end | ||
| rescue Exception => e # rubocop:disable Lint/RescueException | ||
| logger&.error { "debug-socket-error=#{e.inspect} DebugSocket is broken now path=#{@path} backtrace=#{e.backtrace.inspect}" } |
There was a problem hiding this comment.
Should the message inform the user to restart the connection or try another socket?
There was a problem hiding this comment.
There is no other socket. This message means the debug socket server is broken and the process would need to be restarted. This is a fatal (for the debug socket at least) error.
There was a problem hiding this comment.
I don't know if we have ever seen this in practice. BUT I noticed it when testing. When I typo-ed a line, there were a LOT of logs spewing the error because we had a rescue Exception inside a loop. I've now changed it to have a rescue Exception on the eval above (we don't want user input to disrupt anything). But we shouldn't be rescuing SyntaxErrors in this file itself.
Now, we only allow 20 errors that aren't cause by the eval itself, e.g. a socket error can happen if the client writes to the socket and then closes really quickly before it has time to respond. I think that is ok. I don't know how to distinguish "there's something wrong with the socket and we shouldn't infinite loop spewing errors" vs "the client did something wrong and we should rescue the error and keep going". I think it's better to fail VERY conservatively for a debug tool. I never want DebugSocket to interrupt or negatively impact a running process (of course you can do that yourself by sending exit as your command, but don't do that).
lib/debug_socket.rb
Outdated
| rescue Exception => e # rubocop:disable Lint/RescueException | ||
| logger&.error "debug-socket-error=callback unsuccessful: #{e.inspect} for #{input.inspect} path=#{@path} backtrace=#{e.backtrace.inspect}" | ||
| end | ||
|
|
There was a problem hiding this comment.
Might be worth adding a commend explaining why this is being introduced or what we're trying to avoid.
There was a problem hiding this comment.
Good call. I think I'm going to move it into it's own empty module in fact.
nerdrew
force-pushed
the
lazarus/isolated-eval
branch
from
7b90edb to
80d36f6
Compare
| module Commands | ||
| # When running `eval`, we don't want the input to overwrite local variables etc. `eval` runs in the current scope, | ||
| # so we have an empty scope here that runs in a module that only has other shortcut commands the client might want | ||
| # to run. |
|
Not sure if its worth updating the CHANGELOG with this change. Most is encapsulated in the change I mentioned. |
nerdrew
force-pushed
the
lazarus/isolated-eval
branch
from
80d36f6 to
8a72747
Compare
Ah, I should add a changelog entry. This is a big change (in terms of lines of code changed). |
- update rubocop, and fix all rubocop failures - Refactor specs - add github actions CI - remove travis CI
nerdrew
force-pushed
the
lazarus/isolated-eval
branch
from
8a72747 to
244d54e
Compare
