Please use compiled regular exception

Could you explain the rationale for this?

Compiled regexs are faster, isn't it?

Yes, but only if they are cached properly so they aren't recompiled every time. Python does some internal caching for regular expressions already, so unless we need a lot of regular expressions, the internal cache should cover most use cases.

In fact, if we don't cache the regex and compile it every time we need it, we actually degrade performance compared to having Python (or the re module) handle caching automatically.

I propose to cache regexps at module level, like [here](https://github.com/aio-libs/aiohttp/blob/master/aiohttp/helpers.py#L452
re module has 512 slots for LRU cache of compiled regexps.
Thus the program may degrade performance if other code uses non-complied exceptions for solving own tasks. If the total regexps number exceeds 512 -- re will recompile them constantly.

This function will only be run once for every multipart document sent, so I believe the performance impact will be minimal at bestworst, but we can do it anyway if that's your standard.

Thanks.

1. It is the aiohttp standard.
2. The function runs once per multipart document but for high-load site it could clash with other handlers and their functionality.

Compile regex

Why bytes here in and out?

The HTTP spec operates on bytes, which is why I designed this helper function to follow that. The only reason why we need to convert back to a string is because the underlying Payload API requires a string for header values. Or at least I think so. If it doesn't, I'll be glad to remove the conversion back to a unicode string.

I could of course special-case the function for just our situation here, but it should go into multipart.py then.

Also, we need to use bytes in the actual multipart body, so we should try to convert exactly that onto a potentially quoted parameter value.

Because it looks strange. We take string boundary, turns it into bytes, and then turn it back to stirng to apply formatting. This wouldn't be a bottleneck for sure, but looks like useless work.

What is the case for binary boundary input?

It is true to how it is actually used and defined in the spec. In this particular situations, users don't get more value out of it because we have to convert the boundary back to a unicode string for the Payload header. Otherwise characters above 0x80 could be used inside the boundary.

Ideally, all headers would work on byte strings (only or additionally), but I didn't consider this to be in the scope of this PR.

*bytes above 0x80 could be used. HTTP is an ascii-readable byte-based protocol.

If this if-else still exists, would be good to move default boundary generation here into another branch. No point to do isinstance check if boundary passed as None since we know that it will not be a bytes.

It would complicate (or at least worsen code beauty) of the next part when we try to catch encoding errors.

I would recommend to minimize exception catch scope.

The line is not covered by tests

I think you mean re.search. Because match will not match anything if string starts with valid chars.

Good call. Fixed that and another issue with the previous regex not considering that $ also matches before a newline.

How about boundary=test\n? (;

The type bothers me.
We pass optional str value into constructor but the property is byteish.
Maybe the method should return self._boundary.decode('ascii')?
It is a breaking change but we could do it in aiohttp 3.0.
Amount of affected user code is very low IMHO.

This behavior is unchanged. I just documented it.

https://github.com/aio-libs/aiohttp/pull/2545/files#diff-235eeadd1960735e1191879aa500b7b7L637

Yes, I see.
But maybe now is the time for behavior changing?
Current one is confusing a little.

Well, imo everything should be saved as byte-strings anyway (as I explained before), so having the boundary as a byte-string doesn't seem far off. It's only used that way, too, so either we'd add an additional property that converts the boundary from a unicode to a byte string, or we encode it at every point we need it (not a fan).

I'd say we leave this for now. I would rather spend time to convert all header strings to byte-strings rather than unwrapping the boundary here.

I fear it was bytes all the time. Let's change that for 3.0! This could be done with separate PR with breaking change notice.

Ok, let's merge.
Technically single Pull Request can have several records in CHANGES/ but I'm ok with separate PR.

@FichteFoll I have a feeling that all headers should be either strings or bytes.
All means really all, including request.headers etc.
This change in too intrusive, the ship has sailed many years ago.
Also I think 99% aiohttp users are happy with string headers but byte header values will confuse them.

I agree that this should ideally be standardized on a single header type. For compatibility, all unicode strings could be converted to byte-strings using ASCII encoding, so that the outgoing API doesn't change much except that it allows byte-strings for all headers too.

However, this goes way beyond the scope of this PR and would be a large architectural consideration that I'm sure you're going to have a much better time deciding on. It might be something worth considering for the next major release, i.e. 3.0, as long as someone puts in the work.