Skip to content
This repository has been archived by the owner on Dec 10, 2018. It is now read-only.

C framed transport should possibly not flush? #74

Closed
eevee opened this issue Jan 10, 2015 · 6 comments
Closed

C framed transport should possibly not flush? #74

eevee opened this issue Jan 10, 2015 · 6 comments

Comments

@eevee
Copy link

eevee commented Jan 10, 2015

We're seeing a problem where a Thrift server (using upstream Apache's implementation, I believe in Java) hangs when we try to send a request bigger than 4096 bytes.

I tracked the problem down to this line in TCyBufferedTransport. We're using framing, and this line causes a single struct to be broken into two frames, because TFramedTransport creates a new frame on every flush. Presumably the server thinks it should be getting more data, but never does. I don't have access to the server, so I'm not quite sure why.

If I remove that line, everything works fine. (The call to grow would need fixing, but in my case it happens to work.) If I use TBufferedTransport instead, which doesn't need to grow a fixed-size buffer, everything works fine. Either way, the whole message is sent as a single frame.

I'm not sure who's actually at fault here — there doesn't seem to be any documentation of any of the Thrift wire formats, so I have no idea when it's legal to start a new frame. I do notice that both Apache and thriftpy will read incorrectly if they try to read a single contiguous block that spans multiple frames, but that's not what's happening here.

@lxyu
Copy link
Contributor

lxyu commented Feb 9, 2015

hangs when we try to send a request bigger than 4096 bytes

Is the #73 "Fixed framed protocol for large frame sizes" fixed your problem?

@eevee
Copy link
Author

eevee commented Feb 11, 2015

No, the server end isn't using thriftpy. But if this is a server problem then I can see about getting us upgraded, or something.

@lxyu
Copy link
Contributor

lxyu commented Feb 13, 2015

I'm not sure where the problem is too. We're not using the framed transport ourself internally. It would be great if you can give a simplified reproduce procedure. : )

@mikekap
Copy link
Contributor

mikekap commented Feb 15, 2015

On a sidenote: thriftpy does correctly do reads that span frame boundaries - but not if you use TFramedTransport directly. The T(Cy)FramedTransportFactory wraps TFramedTransport in a TBufferedTransport which will call TFramedTransport.read as many times as necessary.

It sounds like the problem is the java framed transport isn't correctly handling multi-frame messages (perhaps thriftpy is a little eager about flushing). Are you on the latest java apache thrift implementation?

@eevee
Copy link
Author

eevee commented Feb 15, 2015

That is an excellent question and I have no idea. It's an ancient service at work that no one seems terribly confident about, and this quality Facebook software doesn't have a manpage and doesn't understand --version. I wouldn't be surprised if it were a very old Thrift, though; before I started porting us to thriftpy, we were using an Apache Thrift that appears to predate the first stable release, somehow. I'll try to figure it out soon. :)

I remember looking through the (current) Apache source and getting the impression that its read code would have the same problem, because it never reads more than one frame ahead or something, which is why I wasn't sure whether thriftpy was correct or not. But I never actually tested that.

@lxyu
Copy link
Contributor

lxyu commented Jan 21, 2016

Close as more info needed to solve it. Feel free to reopen.

@lxyu lxyu closed this as completed Jan 21, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants