Repository: tornadoweb/tornado

Issue:
Redefining options that contain an underscore do not raise exceptions, from 4.3.0
From the release notes in 4.3.0: Dashes and underscores are now fully interchangeable in option names.

Discovered issue using Tornado 4.4.1 in Python 2.7 on Mac OS and Linux.

Internally all options with underscores are converted to dashes e.g. genoa_smtp_host is converted to genoa-smtp-host.

looking at the following code one would expect this to raise an exception:
```
options.define("genoa_smtp_host", 'localhost', help="SMTP server address")
options.define("genoa_smtp_host", 'localhost', help="SMTP server address")
```
However it does not, running the next two lines does raise an exception:
```
options.define("genoa_smtp_host", 'localhost', help="SMTP server address")
options.define("genoa-smtp-host", 'localhost', help="SMTP server address")
```

**Proposed solution:**

The first few lines in OptionParser.define() are: 
```
        if name in self._options:
            raise Error("Option %r already defined in %s" %
                        (name, self._options[name].file_name))
```
I suggest the following:
```
        normalized = self._normalize_name(name)
        if normalized in self._options:
            raise Error("Option %r already defined in %s" %
                        (name, self._options[normalized].file_name))
```

One consideration for the implementor of the solution to this issue, is the description of the error. It could be confusing to a developer/user of Tornado to see an error description as "Option 'genoa-smtp-host' already defined in my_module.py" when the option name they are using is genoa_smtp_host.


  




Discussion:

Patch:
diff --git a/tornado/options.py b/tornado/options.py
index 0a72cc65..707fbd35 100644
--- a/tornado/options.py
+++ b/tornado/options.py
@@ -223,9 +223,10 @@ class OptionParser(object):
         override options set earlier on the command line, but can be overridden
         by later flags.
         """
-        if name in self._options:
+        normalized = self._normalize_name(name)
+        if normalized in self._options:
             raise Error("Option %r already defined in %s" %
-                        (name, self._options[name].file_name))
+                        (normalized, self._options[normalized].file_name))
         frame = sys._getframe(0)
         options_file = frame.f_code.co_filename
 
@@ -247,7 +248,6 @@ class OptionParser(object):
             group_name = group
         else:
             group_name = file_name
-        normalized = self._normalize_name(name)
         option = _Option(name, file_name=file_name,
                          default=default, type=type, help=help,
                          metavar=metavar, multiple=multiple,
diff --git a/tornado/web.py b/tornado/web.py
index 132f42dd..d79889fa 100644
--- a/tornado/web.py
+++ b/tornado/web.py
@@ -319,7 +319,10 @@ class RequestHandler(object):
         if reason is not None:
             self._reason = escape.native_str(reason)
         else:
-            self._reason = httputil.responses.get(status_code, "Unknown")
+            try:
+                self._reason = httputil.responses[status_code]
+            except KeyError:
+                raise ValueError("unknown status code %d" % status_code)
 
     def get_status(self):
         """Returns the status code for our response."""
@@ -1558,7 +1561,11 @@ class RequestHandler(object):
             # send a response.
             return
         if isinstance(e, HTTPError):
-            self.send_error(e.status_code, exc_info=sys.exc_info())
+            if e.status_code not in httputil.responses and not e.reason:
+                gen_log.error("Bad HTTP status code: %d", e.status_code)
+                self.send_error(500, exc_info=sys.exc_info())
+            else:
+                self.send_error(e.status_code, exc_info=sys.exc_info())
         else:
             self.send_error(500, exc_info=sys.exc_info())
 


Test patch:
diff --git a/tornado/test/options_test.py b/tornado/test/options_test.py
index bafeea6f..1a0ac8fb 100644
--- a/tornado/test/options_test.py
+++ b/tornado/test/options_test.py
@@ -7,7 +7,7 @@ import sys
 
 from tornado.options import OptionParser, Error
 from tornado.util import basestring_type, PY3
-from tornado.test.util import unittest
+from tornado.test.util import unittest, subTest
 
 if PY3:
     from io import StringIO
@@ -232,6 +232,24 @@ class OptionsTest(unittest.TestCase):
         self.assertRegexpMatches(str(cm.exception),
                                  'Option.*foo.*already defined')
 
+    def test_error_redefine_underscore(self):
+        # Ensure that the dash/underscore normalization doesn't
+        # interfere with the redefinition error.
+        tests = [
+            ('foo-bar', 'foo-bar'),
+            ('foo_bar', 'foo_bar'),
+            ('foo-bar', 'foo_bar'),
+            ('foo_bar', 'foo-bar'),
+            ]
+        for a, b in tests:
+            with subTest(self, a=a, b=b):
+                options = OptionParser()
+                options.define(a)
+                with self.assertRaises(Error) as cm:
+                    options.define(b)
+                self.assertRegexpMatches(str(cm.exception),
+                                         'Option.*foo.bar.*already defined')
+
     def test_dash_underscore_cli(self):
         # Dashes and underscores should be interchangeable.
         for defined_name in ['foo-bar', 'foo_bar']:
diff --git a/tornado/test/util.py b/tornado/test/util.py
index 6c032da6..5f534e84 100644
--- a/tornado/test/util.py
+++ b/tornado/test/util.py
@@ -1,5 +1,6 @@
 from __future__ import absolute_import, division, print_function
 
+import contextlib
 import os
 import platform
 import socket
@@ -94,3 +95,15 @@ def is_coverage_running():
         except AttributeError:
             return False
     return mod.startswith('coverage')
+
+
+def subTest(test, *args, **kwargs):
+    """Compatibility shim for unittest.TestCase.subTest.
+
+    Usage: ``with tornado.test.util.subTest(self, x=x):``
+    """
+    try:
+        subTest = test.subTest  # py34+
+    except AttributeError:
+        subTest = contextlib.contextmanager(lambda *a, **kw: (yield))
+    return subTest(*args, **kwargs)
diff --git a/tornado/test/web_test.py b/tornado/test/web_test.py
index de26c42e..d79ea52c 100644
--- a/tornado/test/web_test.py
+++ b/tornado/test/web_test.py
@@ -917,10 +917,6 @@ class ErrorResponseTest(WebTestCase):
             self.assertEqual(response.code, 503)
             self.assertTrue(b"503: Service Unavailable" in response.body)
 
-            response = self.fetch("/default?status=435")
-            self.assertEqual(response.code, 435)
-            self.assertTrue(b"435: Unknown" in response.body)
-
     def test_write_error(self):
         with ExpectLog(app_log, "Uncaught exception"):
             response = self.fetch("/write_error")
@@ -1493,9 +1489,9 @@ class StatusReasonTest(SimpleHandlerTestCase):
         response = self.fetch("/?code=682&reason=Bar")
         self.assertEqual(response.code, 682)
         self.assertEqual(response.reason, "Bar")
-        response = self.fetch("/?code=682")
-        self.assertEqual(response.code, 682)
-        self.assertEqual(response.reason, "Unknown")
+        with ExpectLog(app_log, 'Uncaught exception'):
+            response = self.fetch("/?code=682")
+        self.assertEqual(response.code, 500)
 
 
 @wsgi_safe