You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by Erik Frey <er...@last.fm> on 2008/10/14 14:56:28 UTC
invalid state in TFramed/BufferedTransport
Before I submit a patch to TBuffered/FramedTransport I'd like some input as to
which is the better fix:
Right now, when TFramedTransport's underlying transport fails during a
flush(), TFramedTransport enters an irrecoverable state with unflushed data
in its write buffer.
This can cause disastrous results in a naive client implementation:
try
{
results = client.doSomething(first_param);
}
catch (TTransportException &) { socket.close(); socket.open(host, port); }
results = client.doSomething(second_param);
The second time the client is used, it will send the old buffered request and
interpret the wrong response! We've seen this problem in both the c++ and
php libs.
There are two ways to fix this:
The first is for TFramedTransport to clear its write buffer for a flush
regardless of whether the underlying transport succeeds in its write/flush.
The second is for TFramedTransport's close() method to clear its buffer
regardless of whether the underlying flush() succeeds. In this case, the
implementer will have to close/open the framed transport as well as the
socket, but at least this is clear and documentable behaviour.
Thoughts?
Erik
RE: invalid state in TFramed/BufferedTransport
Posted by Mark Slee <ms...@facebook.com>.
Yep, that sounds like the right approach to me. Now that I think about
it, all methods of TFramed/Buffered transports that call the underlying
transport should have try/catch blocks for TTransportException, and
either handle or cascade the Exception appropriately.
-----Original Message-----
From: Erik Frey [mailto:erikf@last.fm]
Sent: Tuesday, October 14, 2008 10:52 AM
To: thrift-dev@incubator.apache.org
Subject: Re: invalid state in TFramed/BufferedTransport
Very well, then its safe to argue that a failed flush means game over:
I'll submit a patch that forces buffered transports to clear their write
buffers regardless of the flush()'s success.
Erik
On Tuesday 14 October 2008 18:42:26 Mark Slee wrote:
> I think any retry behaviors should be encoded into the semantics of
> the transport. If you want retries, then you should configure the
> transport as such, and a single call to flush() should retry on
> failure if desired. This keeps the error-handling state-management
> confined to the transport internals, and doesn't leak the retry
> abstraction out into application code.
>
> Making repeated calls seems like a slipperly slope. My 2c.
>
> Cheers,
> mcslee
>
> -----Original Message-----
> From: Erik Frey [mailto:erikf@last.fm]
> Sent: Tuesday, October 14, 2008 9:07 AM
> To: thrift-dev@incubator.apache.org
> Cc: Bryan Duxbury
> Subject: Re: invalid state in TFramed/BufferedTransport
>
> Okay -- my only concern is whether the ability to retry is inferred in
> a method like flush(), if a flush() fails, should the transport
> preserve its state so that I can try flushing again?
>
> If so, this fix would break that property.
>
> Erik
>
> On Tuesday 14 October 2008 15:56:20 Bryan Duxbury wrote:
> > I think it makes the most sense for a failed flush() to clear the
> > write buffer. That way, you get the most immediate adjustment
> > towards consistency, and since you're closing, the data is clearly
> > unrecoverable anyway.
> >
> > Another approach would be to initialize the buffer in open().
> >
> > -Bryan
> >
> > On Oct 14, 2008, at 5:56 AM, Erik Frey wrote:
> > > Before I submit a patch to TBuffered/FramedTransport I'd like some
> > > input as to which is the better fix:
> > >
> > > Right now, when TFramedTransport's underlying transport fails
> > > during
> > >
> > > a flush(), TFramedTransport enters an irrecoverable state with
> > > unflushed data in its write buffer.
> > >
> > > This can cause disastrous results in a naive client
implementation:
> > >
> > > try
> > > {
> > > results = client.doSomething(first_param); } catch
> > > (TTransportException &) { socket.close(); socket.open(host, port);
> > > }
> > >
> > > results = client.doSomething(second_param);
> > >
> > > The second time the client is used, it will send the old buffered
> > > request and interpret the wrong response! We've seen this problem
> > > in both the c
> > > ++ and
> > > php libs.
> > >
> > > There are two ways to fix this:
> > >
> > > The first is for TFramedTransport to clear its write buffer for a
> > > flush regardless of whether the underlying transport succeeds in
> > > its
> > >
> > > write/flush.
> > >
> > > The second is for TFramedTransport's close() method to clear its
> > > buffer regardless of whether the underlying flush() succeeds. In
> > > this case, the implementer will have to close/open the framed
> > > transport as well as the socket, but at least this is clear and
> > > documentable behaviour.
> > >
> > > Thoughts?
> > >
> > > Erik
Re: invalid state in TFramed/BufferedTransport
Posted by Erik Frey <er...@last.fm>.
Very well, then its safe to argue that a failed flush means game over: I'll
submit a patch that forces buffered transports to clear their write buffers
regardless of the flush()'s success.
Erik
On Tuesday 14 October 2008 18:42:26 Mark Slee wrote:
> I think any retry behaviors should be encoded into the semantics of the
> transport. If you want retries, then you should configure the transport
> as such, and a single call to flush() should retry on failure if
> desired. This keeps the error-handling state-management confined to the
> transport internals, and doesn't leak the retry abstraction out into
> application code.
>
> Making repeated calls seems like a slipperly slope. My 2c.
>
> Cheers,
> mcslee
>
> -----Original Message-----
> From: Erik Frey [mailto:erikf@last.fm]
> Sent: Tuesday, October 14, 2008 9:07 AM
> To: thrift-dev@incubator.apache.org
> Cc: Bryan Duxbury
> Subject: Re: invalid state in TFramed/BufferedTransport
>
> Okay -- my only concern is whether the ability to retry is inferred in a
> method like flush(), if a flush() fails, should the transport preserve
> its state so that I can try flushing again?
>
> If so, this fix would break that property.
>
> Erik
>
> On Tuesday 14 October 2008 15:56:20 Bryan Duxbury wrote:
> > I think it makes the most sense for a failed flush() to clear the
> > write buffer. That way, you get the most immediate adjustment towards
> > consistency, and since you're closing, the data is clearly
> > unrecoverable anyway.
> >
> > Another approach would be to initialize the buffer in open().
> >
> > -Bryan
> >
> > On Oct 14, 2008, at 5:56 AM, Erik Frey wrote:
> > > Before I submit a patch to TBuffered/FramedTransport I'd like some
> > > input as to which is the better fix:
> > >
> > > Right now, when TFramedTransport's underlying transport fails during
> > >
> > > a flush(), TFramedTransport enters an irrecoverable state with
> > > unflushed data in its write buffer.
> > >
> > > This can cause disastrous results in a naive client implementation:
> > >
> > > try
> > > {
> > > results = client.doSomething(first_param); } catch
> > > (TTransportException &) { socket.close(); socket.open(host, port); }
> > >
> > > results = client.doSomething(second_param);
> > >
> > > The second time the client is used, it will send the old buffered
> > > request and interpret the wrong response! We've seen this problem
> > > in both the c
> > > ++ and
> > > php libs.
> > >
> > > There are two ways to fix this:
> > >
> > > The first is for TFramedTransport to clear its write buffer for a
> > > flush regardless of whether the underlying transport succeeds in its
> > >
> > > write/flush.
> > >
> > > The second is for TFramedTransport's close() method to clear its
> > > buffer regardless of whether the underlying flush() succeeds. In
> > > this case, the implementer will have to close/open the framed
> > > transport as well as the socket, but at least this is clear and
> > > documentable behaviour.
> > >
> > > Thoughts?
> > >
> > > Erik
RE: invalid state in TFramed/BufferedTransport
Posted by Mark Slee <ms...@facebook.com>.
I think any retry behaviors should be encoded into the semantics of the
transport. If you want retries, then you should configure the transport
as such, and a single call to flush() should retry on failure if
desired. This keeps the error-handling state-management confined to the
transport internals, and doesn't leak the retry abstraction out into
application code.
Making repeated calls seems like a slipperly slope. My 2c.
Cheers,
mcslee
-----Original Message-----
From: Erik Frey [mailto:erikf@last.fm]
Sent: Tuesday, October 14, 2008 9:07 AM
To: thrift-dev@incubator.apache.org
Cc: Bryan Duxbury
Subject: Re: invalid state in TFramed/BufferedTransport
Okay -- my only concern is whether the ability to retry is inferred in a
method like flush(), if a flush() fails, should the transport preserve
its state so that I can try flushing again?
If so, this fix would break that property.
Erik
On Tuesday 14 October 2008 15:56:20 Bryan Duxbury wrote:
> I think it makes the most sense for a failed flush() to clear the
> write buffer. That way, you get the most immediate adjustment towards
> consistency, and since you're closing, the data is clearly
> unrecoverable anyway.
>
> Another approach would be to initialize the buffer in open().
>
> -Bryan
>
> On Oct 14, 2008, at 5:56 AM, Erik Frey wrote:
> > Before I submit a patch to TBuffered/FramedTransport I'd like some
> > input as to which is the better fix:
> >
> > Right now, when TFramedTransport's underlying transport fails during
> > a flush(), TFramedTransport enters an irrecoverable state with
> > unflushed data in its write buffer.
> >
> > This can cause disastrous results in a naive client implementation:
> >
> > try
> > {
> > results = client.doSomething(first_param); } catch
> > (TTransportException &) { socket.close(); socket.open(host, port); }
> >
> > results = client.doSomething(second_param);
> >
> > The second time the client is used, it will send the old buffered
> > request and interpret the wrong response! We've seen this problem
> > in both the c
> > ++ and
> > php libs.
> >
> > There are two ways to fix this:
> >
> > The first is for TFramedTransport to clear its write buffer for a
> > flush regardless of whether the underlying transport succeeds in its
> > write/flush.
> >
> > The second is for TFramedTransport's close() method to clear its
> > buffer regardless of whether the underlying flush() succeeds. In
> > this case, the implementer will have to close/open the framed
> > transport as well as the socket, but at least this is clear and
> > documentable behaviour.
> >
> > Thoughts?
> >
> > Erik
Re: invalid state in TFramed/BufferedTransport
Posted by Erik Frey <er...@last.fm>.
Okay -- my only concern is whether the ability to retry is inferred in a
method like flush(), if a flush() fails, should the transport preserve its
state so that I can try flushing again?
If so, this fix would break that property.
Erik
On Tuesday 14 October 2008 15:56:20 Bryan Duxbury wrote:
> I think it makes the most sense for a failed flush() to clear the
> write buffer. That way, you get the most immediate adjustment towards
> consistency, and since you're closing, the data is clearly
> unrecoverable anyway.
>
> Another approach would be to initialize the buffer in open().
>
> -Bryan
>
> On Oct 14, 2008, at 5:56 AM, Erik Frey wrote:
> > Before I submit a patch to TBuffered/FramedTransport I'd like some
> > input as to
> > which is the better fix:
> >
> > Right now, when TFramedTransport's underlying transport fails during a
> > flush(), TFramedTransport enters an irrecoverable state with
> > unflushed data
> > in its write buffer.
> >
> > This can cause disastrous results in a naive client implementation:
> >
> > try
> > {
> > results = client.doSomething(first_param);
> > }
> > catch (TTransportException &) { socket.close(); socket.open(host,
> > port); }
> >
> > results = client.doSomething(second_param);
> >
> > The second time the client is used, it will send the old buffered
> > request and
> > interpret the wrong response! We've seen this problem in both the c
> > ++ and
> > php libs.
> >
> > There are two ways to fix this:
> >
> > The first is for TFramedTransport to clear its write buffer for a
> > flush
> > regardless of whether the underlying transport succeeds in its
> > write/flush.
> >
> > The second is for TFramedTransport's close() method to clear its
> > buffer
> > regardless of whether the underlying flush() succeeds. In this
> > case, the
> > implementer will have to close/open the framed transport as well as
> > the
> > socket, but at least this is clear and documentable behaviour.
> >
> > Thoughts?
> >
> > Erik
Re: invalid state in TFramed/BufferedTransport
Posted by Bryan Duxbury <br...@rapleaf.com>.
I think it makes the most sense for a failed flush() to clear the
write buffer. That way, you get the most immediate adjustment towards
consistency, and since you're closing, the data is clearly
unrecoverable anyway.
Another approach would be to initialize the buffer in open().
-Bryan
On Oct 14, 2008, at 5:56 AM, Erik Frey wrote:
> Before I submit a patch to TBuffered/FramedTransport I'd like some
> input as to
> which is the better fix:
>
> Right now, when TFramedTransport's underlying transport fails during a
> flush(), TFramedTransport enters an irrecoverable state with
> unflushed data
> in its write buffer.
>
> This can cause disastrous results in a naive client implementation:
>
> try
> {
> results = client.doSomething(first_param);
> }
> catch (TTransportException &) { socket.close(); socket.open(host,
> port); }
>
> results = client.doSomething(second_param);
>
> The second time the client is used, it will send the old buffered
> request and
> interpret the wrong response! We've seen this problem in both the c
> ++ and
> php libs.
>
> There are two ways to fix this:
>
> The first is for TFramedTransport to clear its write buffer for a
> flush
> regardless of whether the underlying transport succeeds in its
> write/flush.
>
> The second is for TFramedTransport's close() method to clear its
> buffer
> regardless of whether the underlying flush() succeeds. In this
> case, the
> implementer will have to close/open the framed transport as well as
> the
> socket, but at least this is clear and documentable behaviour.
>
> Thoughts?
>
> Erik