Added fixes for WebSocket issues, and extended FileServer

[nikola]

I have seen error 53 (ECONNABORTED) pop up a few times when sending data over WebSockets. My investigation yields that the error is actually harmless, but still annoying when littering the log, so I've extended the error handling code to cover this gracefully.

Also, on at least one occasion length was an unpacked tuple with 1 item. Unpacking results in correct behavior, i.e. no truncation of data.

The third change is with regards to the default fallback headers in send_file, which always take effect when serving from the FileServer because there was no actual propagation of headers defined elsewhere. More specifically, the default 'max-age=604800' turned out to be an issue in my application, so instead of changing the default I have added a headers= kwarg to FileServer().

[ecdavis]

Thanks for your contribution. I'm adding some quick comments now but I'll take a more in-depth look when I get a chance to.

[nikola]

This check should be in the blocks between lines 1269 and 1279.

length being a tuple must be a result of lines 1269-1279, because in 1267 it's implicitly cast to an integer, that's why I have placed my modification at 1290, as the last check before further usage.

Mutable types should not be used as default values for parameters.

You are correct, of course. Do you want me to PR a fixed version?

[ecdavis]

Re: the length issue. From the Python docs for struct.unpack:

Unpack the string [...] according to the given format. The result is a tuple even if it contains exactly one item.

https://docs.python.org/2/library/struct.html#struct.unpack

So the return value should always be unpacked on lines 1272 and 1278 - also line 1311.

Good catch!

EDIT: Actually, the bug doesn't exist on 1311 because the assignment is indexed. It's worth deciding on a consistent approach to this. Indexing looks nicer but could be more error prone. I'll think about it.

[nikola]

@ecdavis

On mutable dicts, ironically, I was encountering a strange issue a while ago where subsequent requests against pants responded with seemingly random data. After a while I found the root cause: to prevent repeating myself I had put my default header constants into a single module-level dict, and re-used it throughout my routes, i.e. I modified the header dict in each route, which really was the module-level dict.

Unfortunately, it seems that this also happens a lot in pants, e.g. code that reads like this:

class Foo(headers=None):
    if headers is None:
        self.headers = {}
    else:
        self.headers = headers

should rather read:

class Foo(headers=None):
    if headers is None:
        self.headers = {}
    else:
        self.headers = headers.copy()

Of couse pants should not try to hold the user's hands wrt Python's language features, but in this case passing the same header dict will manifest in behavior that obfuscates the real mistake.

[ecdavis]

So, to summarize my comments on this PR:

• ECONNABORTED: I won't be accepting this change - please remove it.
• length: Please add [0] to the end of lines 1272 and 1278 individually rather than adding the new block. We should keep the type of the length variable consistent throughout the method.
• send_file: The default parameter should be None and checked with if not headers (not if headers is None - we want to be consistent through the codebase). The copy() call should be removed. You can add a copy() in HTTPRequest.send_file() (or one of us can do it - either way it should be added). Documentation should be added to the FileServer class for the new headers parameter.

Once you've made these changes please submit a new PR and I'll take a look and hopefully be able to merge. I may be somewhat pedantic about the documentation, so if you'd prefer me to write it myself let me know.

Thanks!

[nikola]

Ok, thanks for the feedback, I'll submit a new PR.