[0.9.1] Red5.getLocalConnection 'corrupted' during connection closing

[Nicolas Leonard]

Here is a HUGE bug I just create a ticket for (http://red5.org/ticket/
733)

--

I finally find what was the underlying bug which was corrupting my
whole app:

To reproduce it:
* Connect 2 clients A and B to a basic red5 application
* A publishes a stream
* B subscribes to the stream
* A disconnects
=> All the events (IStreamAwareScopeHandler.onStreamBroadcastClose,
ISharedObjectListener.onSharedObjectDisconnect ...) called during the
closing of the connection will be called with a
Red5.getConnectionLocal() returning the connection of B instead of A

Reason:
* RTMPConnection.close() first calls Red5.setConnectionLocal() to
create the association
* then it calls streamService.deleteStream() on each streams 'owned'
by the closing connection
* each subscriber to the streams are notified of the disconnection
through PlayEngine.onPipeConnectionEvent and so on
* the status message called RTMPMinaProtocolEncoder.encode() which
first set .... the Red5.setConnectionLocal() to the targeted client

What is the purpose of calling Red5.setConnectionLocal() in the
encoder ? - the comment let me think of a copy/paste bug (it's related
to the decoder)

Potential fixes:
* Remove this call if possible ... I really don't see the point for
that in the encoder
* Re-set the connection after the reset of the streams, in
RTMPConnection.close()

It has been introduced in r3895 (02/11/2009 - "Merged with xuggle
timestamp branch"), I didn't check the history of the branch
(And, by the way, why don't you use the merge feature of svn - it will
be easier to look through the history of merged branches)

Let me know if you want more informations. This bug is critical to me.

Here is the full stack:

Thread [NioProcessor-1] (Suspended (breakpoint at line 96 in Red5))
Red5.setConnectionLocal(IConnection) line: 96
RTMPMinaProtocolEncoder.encode(IoSession, Object,
ProtocolEncoderOutput) line: 51
ProtocolCodecFilter.filterWrite(IoFilter$NextFilter, IoSession,
WriteRequest) line: 298
DefaultIoFilterChain.callPreviousFilterWrite(IoFilterChain$Entry,
IoSession, WriteRequest) line: 506
DefaultIoFilterChain.access$1400(DefaultIoFilterChain, IoFilterChain
$Entry, IoSession, WriteRequest) line: 46
DefaultIoFilterChain$EntryImpl$1.filterWrite(IoSession, WriteRequest)
line: 805
TrafficShapingFilter(IoFilterAdapter).filterWrite(IoFilter
$NextFilter, IoSession, WriteRequest) line: 135
DefaultIoFilterChain.callPreviousFilterWrite(IoFilterChain$Entry,
IoSession, WriteRequest) line: 506
DefaultIoFilterChain.access$1400(DefaultIoFilterChain, IoFilterChain
$Entry, IoSession, WriteRequest) line: 46
DefaultIoFilterChain$EntryImpl$1.filterWrite(IoSession, WriteRequest)
line: 805
DefaultIoFilterChain$TailFilter.filterWrite(IoFilter$NextFilter,
IoSession, WriteRequest) line: 731
DefaultIoFilterChain.callPreviousFilterWrite(IoFilterChain$Entry,
IoSession, WriteRequest) line: 506
DefaultIoFilterChain.fireFilterWrite(WriteRequest) line: 498
NioSocketSession(AbstractIoSession).write(Object, SocketAddress)
line: 428
NioSocketSession(AbstractIoSession).write(Object) line: 369
RTMPMinaConnection.write(Packet) line: 248
Channel.write(IRTMPEvent, int) line: 124
Channel.sendStatus(Status) line: 168
ConnectionConsumer.pushMessage(IPipe, IMessage) line: 118
InMemoryPushPushPipe.pushMessage(IMessage) line: 82
PlayEngine.doPushMessage(AbstractMessage) line: 940
PlayEngine.doPushMessage(Status) line: 930
PlayEngine.sendUnpublishedStatus(IPlayItem) line: 1190
PlayEngine.onPipeConnectionEvent(PipeConnectionEvent) line: 1300

InMemoryPushPushPipe(AbstractPipe).firePipeConnectionEvent(PipeConnectionEvent)
line: 258

InMemoryPushPushPipe(AbstractPipe).fireProviderConnectionEvent(IProvider,
int, Map<String,Object>) line: 248
InMemoryPushPushPipe(AbstractPipe).unsubscribe(IProvider) line: 111
ClientBroadcastStream.close() line: 196
StreamService.deleteStream(IStreamCapableConnection, int) line: 175
RTMPMinaConnection(RTMPConnection).close() line: 596
RTMPMinaConnection.close() line: 76
RTMPHandler(BaseRTMPHandler).connectionClosed(RTMPConnection, RTMP)
line: 219
RTMPMinaIoHandler.sessionClosed(IoSession) line: 113
DefaultIoFilterChain$TailFilter.sessionClosed(IoFilter$NextFilter,
IoSession) line: 662
DefaultIoFilterChain.callNextSessionClosed(IoFilterChain$Entry,
IoSession) line: 395
DefaultIoFilterChain.access$900(DefaultIoFilterChain, IoFilterChain
$Entry, IoSession) line: 46
DefaultIoFilterChain$EntryImpl$1.sessionClosed(IoSession) line: 778
TrafficShapingFilter(IoFilterAdapter).sessionClosed(IoFilter
$NextFilter, IoSession) line: 95
DefaultIoFilterChain.callNextSessionClosed(IoFilterChain$Entry,
IoSession) line: 395
DefaultIoFilterChain.access$900(DefaultIoFilterChain, IoFilterChain
$Entry, IoSession) line: 46
DefaultIoFilterChain$EntryImpl$1.sessionClosed(IoSession) line: 778
ProtocolCodecFilter.sessionClosed(IoFilter$NextFilter, IoSession)
line: 345
DefaultIoFilterChain.callNextSessionClosed(IoFilterChain$Entry,
IoSession) line: 395
DefaultIoFilterChain.access$900(DefaultIoFilterChain, IoFilterChain
$Entry, IoSession) line: 46
DefaultIoFilterChain$EntryImpl$1.sessionClosed(IoSession) line: 778
DefaultIoFilterChain
$HeadFilter(IoFilterAdapter).sessionClosed(IoFilter$NextFilter,
IoSession) line: 95
DefaultIoFilterChain.callNextSessionClosed(IoFilterChain$Entry,
IoSession) line: 395
DefaultIoFilterChain.fireSessionClosed() line: 388
IoServiceListenerSupport.fireSessionDestroyed(IoSession) line: 210
NioProcessor(AbstractPollingIoProcessor<T>).removeNow(T) line: 535
NioProcessor(AbstractPollingIoProcessor<T>).removeSessions() line:
497
AbstractPollingIoProcessor<T>.access$600(AbstractPollingIoProcessor)
line: 61
AbstractPollingIoProcessor$Processor.run() line: 974
NamePreservingRunnable.run() line: 64
ThreadPoolExecutor$Worker.runTask(Runnable) line: 886
ThreadPoolExecutor$Worker.run() line: 908
Thread.run() line: 619

[Nicolas Leonard]

No reaction or suggestion for the fix ? ... Am I the only one to find
it critical ??

[Gergely Zayzon]

Nicolas,

Red5.getConnectionLocal() is not reliable in the sense that it certainly
returns the current connection, there are several other situations where
you can end up in the same trouble. If you've spoted the problem in this
particular case, submit a patch to the list, and it will be merged into
future releases after reviewing, otherwise you can work around the
problem using other methods for determining the current connection, like
fetching the IConnection from the passed stream (in the case of
onStreamBroadcastClose).

Regards,
Gergely Zayzon

[Nicolas Leonard]

Gergely,

What are those 'several other situations' where do you think
Red5.getConnectionLocal() could not be reliable ?
This function is crucial, at least for me, so it does scare me to know
that it's not so reliable as I thought it was.

For my problem, I've applied a local simple patch to fix it, but ugly
in a way - here it is:
--- src/org/red5/server/net/rtmp/RTMPConnection.java 2010-03-29
18:00:25.420027200 +0200
+++ src/org/red5/server/net/rtmp/RTMPConnection.java 2010-03-31
21:54:10.359375000 +0200
@@ -599,6 +599,7 @@
}
streams.clear();
}
+ Red5.setConnectionLocal(this);
channels.clear();
super.close();
}

It works in my specific case, but it's pretty ugly - I would prefer:
* To remove the Red5.setConnectionLocal() in
RTMPMinaProtocolEncoder.encode(), but I need some advice from the Red5
core team, because I cannot guess the side effects
* To change totally the way the local IConnection is "spread" - I'm
definitely not fond of the way it's done, using Thread Local Storage -
it makes me think of global variables and I hate global
variables ..... but it's a pretty intrusive patch (and destructive for
existing applications), and it's more a question of design ...

You're right, I could have use some work arounds in
onStreamBroadcastClose(), but I didn't find any in
onSharedObjectDisconnect() callbacks.

Thanks for your answer,

Nicolas

[Andy Shaules]

Red5.getConnectionLocal() is unreliable in cases where the Connection is
potentially 'not present' such as in the appDisconnect or roomLeave.

However even in the case where the client disconnects, the only operations
you loose is the ability to call a method on the parting client.

Descibing the behavior of your code without posting the client as3 code or
any application code showing your 'id' routine, fills me with doubt.

Gergely,

Thanks for your answer,

Nicolas

--
To unsubscribe, reply using "remove me" as the subject.

[Andy Shaules]

Ok, I looked at an application that uses appdisconnect for critical logic.

And since appDisconnect provides the IConnection as a method parameter, what
is the purpose of 'Red5.getConnectionLocal' ?

The Connection wont even exist half the time in this method.

It may even take 30 seconds befor it is called.

So you have all of these onDisconnection events in your
scopeawarestreamhandler firing off, when IMHO, you have only one main event
to catch.

And still knowing very little of your app logic I can only suppose so much.

Think about what might be happening at the moment your client disconnects.
There is no gurantee of any particular order of events, however you do have
'appDisconnect' with the IConnection handed to you.

I would expect very unreliable behavior in Red5.getConectionLocal, since it
is no longer local in the life cycle of the object.

----- Original Message -----
From: "Nicolas Leonard" <leonard...@gmail.com>
To: "red5" <red5in...@googlegroups.com>
Sent: Friday, April 09, 2010 8:39 AM
Subject: [Red5] Re: Red5.getLocalConnection 'corrupted' during connection
closing

Gergely,

Thanks for your answer,

Nicolas

[Nicolas Leonard]

Whats doubts ??? I've described the context, given the stack,
explained the problem and found potentials solutions, want do you want
more than that ?
I can reproduce at a 100% probability ... either I won't be so
precise.

Again, if you really need more evidences than that, I can create an
AS3 project and a simple red5 application .. but only of it's
necessary ...

Anyway, I'll try to describe the problem simply: I have shared objects
that are 'owned' by the client which create it. The object is of
course shared with all the clients, and I need to know (from this
shared object point of view), when the owner disconnects to execute
some tasks.

So I registered a shared object listener with
ISharedObjectBase.addSharedObjectListener(), and do my stuff in the
onSharedObjectDisconnect() callback.
I know that the IConnection has a limited lifetime, of course, I don't
send any data to the disconnected client, I just need to know, in THIS
callback, that THIS client (and therefore THIS connection) is the one
disconnecting, and I expect Red5.getConnectionLocal() to give me that.

And it works perfectly, expect in the condition I wrote in my first
message : when the connection which's disconnecting has some published
streams associated which need to be closed and notified to the
subscribed clients .... because Red5.getConnectionLocal() is overriden
in RTMPMinaProtocolEncoder.encode()

It's not a matter of appDisconnect().

And I can not provide any AS3 standalone code to reproduce the code
right now, I spend HOURS to find it tracing my application. Look at
the stack, it's perfectly clear:
- RTMPConnection.close() - Line 588
Red5.setConnectionLocal(this); <== GOOD ONE
- RTMPConnection.close() - Line 596
streamService.deleteStream() <== Called only when there are
stream published
- AbstractPipe.unsubscribe - Line 111 <=
Send unpublished status to all clients subscribed
- RTMPMinaProtocolEncoder.encode() - Line 51
Red5.setConnectionLocal() <== Override the connection with the
client
target of the message, for no apparent reason (look at the
comment
...) and leave it crappy

And as I wrote in my bug report, this bug wasn't there in 0.8.

> To unsubscribe, reply using "remove me"...
>
> plus de détails »

[Gergely Zayzon]

Nicolas,

It usually works, but as a nature of such a "magic" function it can
cause odities, which is in my terminology means unreliable. But for
example I always use it in AS invoked methods, and that works fine, even
if they're chained in one way or other, so you don't need to be scared
too much. Maybe cautious? :-)

What I personally encountered so far is if you disconnect a client from
a method invoked from AS code, then Red5.getConnectionLocal() in
appDisconnect() returns the wrong connection. That's how i remember, but
maybe there was some more things involved. I could redesign my code
fairly easy to circumvent the problem, but it was a good lesson to make
me avoid it whenever it's possible.

onSharedObjectDisconnect() does not seem to have anything related to the
disconnecting user indeed, in that case you either stick with
Red5.getConnectionLocal(), or redesign your code that it will not need
the connection at this point anymore. Waiting for a release with a fix
for such a wired-in thing sound pointless, as the team is focusing on
features instead of stability or refactoring...

Regards,
Gergely Zayzon

[Andy Shaules]

Yeah, I see how it happens in the stack there. It is pretty clear now to me.

I could suggest only to deal with your application logic for disconnting
clients in 'appDisconnect' where the specific IConnection is passed to you
as a parameter. At that point you can retieve any attributes to identify the
client resources and provide appropriate action for clean-up.

I generally have 100% reliabilbity in my join/part logic doing it this way,
including scenarios where each user has one shared object and each user in
the same scope all subscribe to each others objects.

Using join/part mechanism at a single gateway event to construct andf
destroy said resources has been very good to me. Stream handlers are not any
more complicated provided you listen for status events and release streams
as needed.

----- Original Message -----
From: "Gergely Zayzon" <ger...@harras.be>
To: <red5in...@googlegroups.com>
Sent: Friday, April 09, 2010 4:10 PM
Subject: Re: [Red5] Re: Red5.getLocalConnection 'corrupted' during
connection closing

> --
> To unsubscribe, reply using "remove me" as the subject.

[Nicolas Leonard]

:) Thanks I will be cautious.

Yes I could redesign my code, but it would not be efficient in the way
that I was using this function onSharedObjectDisconnect() to manage
either the disconnection of a client (ie. a connection closed) and an
explicit disconnection of the client from this shared object (ie. AS3
SharedObject.close()) (it can happen for dynamic objects created on
the client side)
It's true that I could separate this:
* use onAppDisconnect() or onLeaveRoom() to manage the first case
* implement and use a RPC function to manage the 2nd case, through
an explicit call from the client

... but I prefer the bug to be fixed (anyway it's fixed in my local
repository) ..

I didn't know that the team was focusing on features instead of
stability/refactoring ... too bad .. I really don't see the point to
do that that way ... but I'm not involved in the Red5 development, so
I've to live with it :(

Unfortunately for me, I'm using Red5 for production servers .. and I
don't think I'm the only one ... so I guess stability and non-
regression is just a must have for a lot of people ...

Thanks

> >>>>           DefaultIoFilterChain.callNextSessionClosed(IoFilterChain$Entry,...
>
> plus de détails »