support 2.3 on CI #1
Conversation
lib/protocol/http1/connection.rb
Outdated
|
|
||
| def read_line? | ||
| @stream.gets(CRLF, chomp: true) | ||
| @stream.gets(CRLF).chomp |
There was a problem hiding this comment.
The problem with this is it allocates extra strings and this breaks memory usage assumptions. I'll need to confirm this is going to work in light of the memory limit specs we have.
There was a problem hiding this comment.
I checked and result is here https://gist.github.com/ganmacs/52fbcabdc1311d43dcd730442672a7a9.
The problem with this is it allocates extra strings and this breaks memory usage assumptions
it's certainly so but, @stream.gets(CRLF, chomp: true) creates a new Hash instance and it allocates more memory than @stream.gets(CRLF).chomp ver.
There was a problem hiding this comment.
Yes, you are right, at the time I considered this.
Fortunately, it only allocates a hash when @stream is actually a Ruby IO instance. However, for Async::IO::Stream because gets is pure Ruby, it doesn't allocate a hash. It's very tricky to get right and I agree it's kind of crappy edge case.
I'm happy to work on supporting 2.3 as a baseline, but I just need to review how the entire thing fits together. It might even be the case that we just version detect, e.g. if RUBY_VERSION <= "2.3" use .chomp
There was a problem hiding this comment.
In pure ruby implementation of streams, @stream.gets(CRLF, chomp: true) is more efficient too.
There was a problem hiding this comment.
e.g. if RUBY_VERSION <= "2.3" use .chomp
if you are okay with it. i'm ok too 👍
43802f0
the following implementation is the best, but it will block and never
return. I don't know why it's occured...
```
class IO
def gets(*args, chomp: false)
chomp ? super(*args).chomp : super(*args)
end
end
```
|
@ioquatix what do you think of this code? |
|
I did this in a way which allowed me to keep the code isolated and easy to remove in the future. Thanks for this PR and the related discussion, and your efforts. Please keep it coming! |
|
I also reviewed |
2 participants
ref socketry/protocol-http#5 (comment)