Fix exception causes in server.py

[cool-RR]

I recently went over Matplotlib, Pandas and NumPy, fixing a small mistake in the way that Python 3's exception chaining is used. If you're interested, I can do it here too. I've done it on just one file right now.

The mistake is this: In some parts of the code, an exception is being caught and replaced with a more user-friendly error. In these cases the syntax raise new_error from old_error needs to be used.

Python 3's exception chaining means it shows not only the traceback of the current exception, but that of the original exception (and possibly more.) This is regardless of raise from. The usage of raise from tells Python to put a more accurate message between the tracebacks. Instead of this:

During handling of the above exception, another exception occurred:

You'll get this:

The above exception was the direct cause of the following exception:

The first is inaccurate, because it signifies a bug in the exception-handling code itself, which is a separate situation than wrapping an exception.

Let me know what you think!

[cool-RR]

Looks like the mypy error in the CI is unrelated to my change.

[aaugustin]

raise ... from None would be even better in this case, wouldn't it?

[aaugustin]

The mypy behavior changes are a constant source of happiness.

In this case:

• it looks like a bug was fixed, so I no longer need to tell mypy to ignore things it got wrong; progress!
• we're hit by a regression caused by a change in typeshed: signal: improve version availability python/typeshed#4079; perhaps making the code in websockets conditional on sys.platform, like the typeshed code, could help.

[aaugustin]

Coming back to the original issue, I'm in favor of making changes wherever websockets changes the type of an exception.

We should choose between raise ... from exc or raise ... from None depending on whether the original exception is likely to add useful information for debugging.

[cool-RR]

Regarding the mypy issue: I'll rebase and see what happens.

Regarding from None: I personally hate it, and I think that when people are debugging they have a very different mindset than when they are writing code. When you're debugging, and you get a very limited amount of information that may have been copy-pasted in an email by a non-technical person, you're very desperate for the information.

In this case, I would have wanted to see the cast(Origin, headers.get("Origin")) traceback. I would maybe get a hint on what happened. If it's a dead end for exploration, I at least want to know its content before I rule it out as a dead end.

Another issue is that some error-reporting systems show not only the traceback, but all the local variables on each of these levels. These supposedly irrelevant levels could have variables that make it clearer what this exception is about.

My policy is to never add from None. I'm okay with you marking the places where you'd prefer from None, and I'll just remove them from my PR(s) and keep the lines we agree about, so you could do the from None on a separate PR.

[aaugustin]

I fixed the mypy failures in master just now.

[aaugustin]

Style detail: I'd rather use as exc / from exc than as e / from e, for consistency with the current style.

Happy to look at a patch fixing other instances besides this one :-)

[cool-RR]

Fixed and rebased. Would you be okay with the other instances being fixed in a separate PR after this one is merged?

[codecov]

Codecov Report

Merging #776 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #776   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           31        29    -2     
  Lines         4433      4388   -45     
  Branches       409       406    -3     
=========================================
- Hits          4433      4388   -45     

Impacted Files	Coverage Δ
src/websockets/server.py	100.00% <100.00%> (ø)
tests/__init__.py
tests/utils.py

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24a77de...02199b9. Read the comment docs.

[cool-RR]

Cool :) Expect another PR soon.