Skip to content
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

Do not report an error in websocket_log when closing a VNC connection #18342

Merged
merged 1 commit into from
Jan 9, 2019

Conversation

skateman
Copy link
Member

@skateman skateman commented Jan 9, 2019

When closing a VNC console, the transmission block (1) can throw an IO::WaitWritable or with an even higher probability an IO::WaitReadable exception. These are not inherited from the IOError, so the cleanup will not treat this a closed exception (2) but as an error (3).

begin
  # transmission (1)
rescue IOError
  # clean up closed connection (2)
rescue StandardError => ex
  # clean up error (3)
end

The cleanup function is the same for both (2) and (3), the only difference is the way how we report them into the websocket log, That's the problem I'm trying to fix here by adding the 2 exception classes into (2).

@miq-bot add_label bug, providers/console, hammer/no
@miq-bot add_reviewer @djberg96, @kbrock @jrafanie

@miq-bot
Copy link
Member

miq-bot commented Jan 9, 2019

@skateman 'djberg96, kbrock jrafanie' is an invalid reviewer, ignoring...

@skateman
Copy link
Member Author

skateman commented Jan 9, 2019

EAGAIN
@miq-bot add_reviewer @kbrock
@miq-bot add_reviewer @jrafanie
@miq-bot add_reviewer @djberg96

@miq-bot
Copy link
Member

miq-bot commented Jan 9, 2019

Checked commit skateman@2d1204f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

@djberg96
Copy link
Contributor

djberg96 commented Jan 9, 2019

Looking at the exception hierarchy, those are subclasses of Errno::EAGAIN, but even with your changes it would still not pickup Errno::EINPROGRESS errors. Are those possible? If so, I would suggest being even more generic and rescuing SystemCallError instead.

@skateman
Copy link
Member Author

skateman commented Jan 9, 2019

@djberg96 where are you looking at the hierarchy? Just because:

IO::WaitReadable < Errno::EAGAIN
=> nil

I think the SystemCallError is a little too much, originally I got an IO::EAGAINWaitReadable exception.

@jrafanie
Copy link
Member

jrafanie commented Jan 9, 2019

Looking at IO select docs, ( I believe that's what you're doing here, right?), the two additional exceptions seems appropriate.

@skateman
Copy link
Member Author

skateman commented Jan 9, 2019

@jrafanie we're not really calling IO.select anymore (except for OSX), but the code snippet in that comment seems just right. But if you send me a C code, I read it 😆 here are the related lines:

All the transmissions for any protocol combination eventually end up calling those two methods.

@jrafanie jrafanie merged commit a3a2159 into ManageIQ:master Jan 9, 2019
@jrafanie jrafanie self-assigned this Jan 9, 2019
@jrafanie jrafanie added this to the Sprint 103 Ending Jan 21, 2019 milestone Jan 9, 2019
@skateman skateman deleted the websocket-server-exception branch January 10, 2019 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants