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

ignore exceptions on Websocket dispatch parsing #156

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arBmind
Copy link

@arBmind arBmind commented Jan 9, 2016

For me this kept crasching in the HTTP::parser.
This hack makes this module usable for me.

@e2
Copy link
Contributor

e2 commented Jan 9, 2016

I can't accept that patch - especially not the empty rescue.

Also, for handling an error like this, a test case is needed - so what data did you get that caused the error?

@arBmind
Copy link
Author

arBmind commented Jan 10, 2016

My setup:

  • windows host
  • ubuntu 14.04 vm on virtualbox (vagrant)
  • guard runs on a non-shared folder
  • livereload port is forwarded to the host (next to the webserver)
  • Chrome is used to access the webserver and the livereload

Experience:

  • telnet to the livereload port works but closing the session, crashes the guard-livereload
  • activating the livereload browser extension works, but after one or two changes the livereload crashes.

As I said it's a hack, but it allows me to use this reliably.

That parsing of user input may fail is not really an exception - it should be expected. At least not one that should crash the service.

There is no error handling code in this file, so I went the lazy route.
Feel free to adapt, what seems right. It may also solve some of the open issues. I have not checked.
I would be very happy to see a working solution.

@e2
Copy link
Contributor

e2 commented Jan 10, 2016

The most frustrating issues are when something doesn't work and there's no error. Especially for users who aren't savvy enough to debug web sockets, etc.

Any error handling would have to be covered by specs, so it's best to use a real-life scenario. This includes testing for error messages properly shown.

Also, It's better to catch specific exceptions instead of all exceptions - and given most livereload users are designers, it's fair to "reinterpret" exceptions to tell people what could be wrong and how to fix it.

So what is the exact input you're getting that's causing the failure? Why is such an error "normal"?

If I can't reproduce the problem, there's a high chance this will either be a back-and-forth between you and me, or, many users will get very frustrated by "reloading not working" and us having no output to find the cause.

At least paste the data causing the failure - adapt it to prevent it having sensitive info if needed.

@AlexWayfer
Copy link

So what is the exact input you're getting that's causing the failure? Why is such an error "normal"?

I've catched it and described in #179.

I agree that we should not just add an empty rescue, and we should write tests and behavior for this.

But the situation is terrible for me as developer for different devices.

petebytes added a commit to petebytes/guard-livereload that referenced this pull request Jun 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants