Add ReadLimit to Option structs

[ash2k]

Would it be possible to add a field to AcceptOptions and to DialOptions that sets the read limit on the created connection? It's not as convenient to have to call the SetReadLimit(). If the field is 0 then a default limit can be applied.

Is there a need to change the limit dynamically? I mean, why is it a method (i.e. can be called multiple times) vs a field in those option setting structs?

[nhooyr]

I considered such an option initially but my thinking was that it's possible you'd want to adjust the read limit depending on the user and whether they're verified/logged in but now that I think about it, it does stick out like a sore thumb in the API.

Maybe I should deprecate that method and then add ReadLimit to the options. Will sit on this.

Thanks for the suggestion!

[nhooyr]

I also wanted to avoid duplication in the option structs but I don't think it's a big deal now, the compression options are duplicated anyway and it wouldn't make sense to turn those into a function.

[nhooyr]

Maybe Options to contain the shared options for both Accept/Dial and then DialOptions and AcceptOptions embed Options.

[ash2k]

that it's possible you'd want to adjust the read limit depending on the user and whether they're verified/logged in

That sounds like a very specific requirement that can be met without any library support - just get the stream and wrap it in a custom reader, which is trivial to implement. Anybody actually asked for this?

Maybe Options to contain the shared options for both Accept/Dial and then DialOptions and AcceptOptions embed Options.

👍

[nhooyr]

That sounds like a very specific requirement that can be met without any library support - just get the stream and wrap it in a custom reader, which is trivial to implement. Anybody actually asked for this?

I agree, for such a situation it's best to implement your own message limiting. No one asked. In retrospect it was a mistake compared to putting it Options but it seemed like a good idea at the time to keep things flexible. Also means you can still use wjson and wspb.

The reason read limit is in the library at all is so that a proper error message is written to the peer. Using io.LimitReader for example will return an EOF on the limit being hit which is confusing.

[ash2k]

The reason read limit is in the library at all is so that a proper error message is written to the peer. Using io.LimitReader for example will return an EOF on the limit being hit which is confusing.

That makes sense, but would it not be better to allow for composition and allowing users to use this piece of code when they actually need it? e.g. still provide it in the library, but just as a helper.

I understand that changing things in a backward-incompatible way may not be an option, it's just a thought experiment.

[ash2k]

Another idea: make HTTPClient field in DialOptions an interface to allow to inject custom client implementation/wrapper/mock.

[nhooyr]

That makes sense, but would it not be better to allow for composition and allowing users to use this piece of code when they actually need it? e.g. still provide it in the library, but just as a helper.

Just wouldn't seem right to have websocket.LimitReader.

Another idea: make HTTPClient field in DialOptions an interface to allow to inject custom client implementation/wrapper/mock.

http.Transport is an interface so you can already do that!

[felbit]

Has there been an update to the ReadLimit? I have an active use case for this.