search

jgaskins / grpc

Pure-Crystal implementation of gRPC
MIT License
77 stars 12 forks source link

Setting Content-Type causes an exception #7

Open Serdnad opened 4 years ago

Serdnad commented 4 years ago

I followed the setup instructions in the README, and everything compiles, but as soon as I run the client code with a server running (i.e. every time I fire a request), I get this from the server:

Unhandled exception in spawn: huffman string padding is larger than 7-bits (HTTP2::HPACK::Error)
  from lib/grpc/src/hpack.cr:322:17 in 'string'
  from lib/grpc/src/hpack.cr:64:21 in '->'
  from ../../../../usr/local/Cellar/crystal/0.35.1/src/primitives.cr:255:3 in 'run'

Your example repo was working great out of the box for me though, and the only difference seemed to be the lib versions, so I stepped through each release from v0.1.0 to v0.1.4 of the library, and found that when I remove the content headers by unapplying this commit, it works again, even with the changes in v0.1.4.

If there's any way I can help debug this, I'd be happy to help, though I was able to reproduce this just by creating a new project and following the README. Awesome work btw, thanks for the library.

Quick Update: Don't know what I touched just now, but now the changes in v0.1.4 are giving me a different exception... everything seems to work swell for me up until v0.1.2.

juozasg commented 4 years ago

+1 i'm trying to fix it

jgaskins commented 4 years ago

Gah, sorry, I don't know how I missed the opening of this issue, but I appreciate that y'all have ideas in motion to fix it.

I've honestly only ever communicated to a Crystal gRPC server through the grpc Ruby gem and that's worked so far. The Ruby gem was originally the client I built this shard from because the services that talk to my Crystal gRPC services were originally written in Ruby, but I'd be happy to support other clients.

I honestly can't remember the specific reason I added the content-type header, but something wasn't working without it. It's entirely possible it was something that changed in the Ruby gem.

Serdnad commented 4 years ago

Hey @jgaskins , thanks for sharing that tidbit, it was helpful to know the server's been a bit more thoroughly tested - comparing responses to a Crystal client and BloomRPC seems to confirm that things on the server side are fine.

I dug around the code, and realized that a lot of my issues seemed to go away if I just disabled huffman encoding here. Not an ideal fix, but if I can find a reasonable place to add a param (open to suggestions) to enable/disable it, I'd be happy to open a PR

jgaskins commented 4 years ago

@Serdnad Oh nice catch!. I'll have a look at this in the morning (just got done with a 13-hour road trip), but I wonder if there's a bug in the Huffman encoder/decoder.

SEbbaDK commented 3 years ago

Does anyone have any info about this or does anyone have a fix other than disabling the compression?

jgaskins commented 3 years ago

@SEbbaDK I was actually recently considering removing the H2 implementation from this gem to use a fork of ysbaddaden/http2 that would make it loadable as a dependency (it isn't currently due to the names of some files at the root of the repo). It's a much more complete implementation of H2 and may fix this problem.

Unfortunately, because my current company doesn't use gRPC I haven't had the motivation to finish that experiment yet. I was going to do it for my OpenTelemetry shard but it turns out gRPC isn't required for that, so laziness won out again. 😄

There is a chance, though, that it won't solve the problem since that shard is actually where the Huffman implementation in this shard comes from.