Possible bug in DefaultBroadcaster?

4 messages

Cagatay Civici

Possible bug in DefaultBroadcaster?

Reply Threaded

Some javascript/style in this post has been disabled (why?)

Hi,

I was tackling with a problem of broadcasting to a subset of AtmosphereResources, when using DefaultBroadcaster.broadcast(message) everything is fine but with DefaultBroadcaster.broadcast(message, Set set)

the onStateChange is called with a null AtmosphereResourceEvent.

I'm not a Atmosphere code guru but the push implementation in Broadcast could be the problem;

 protected void push(Entry msg) {
181         AtmosphereResourceEventImpl e = null;
182         if (msg.eventsToPush == null) {
183             for (AtmosphereResource r : events) {
184                 synchronized (r) {
185                     if (r instanceof AtmosphereResourceImpl) {
186                         e = ((AtmosphereResourceImpl)r).event();
187                         e.setMessage(msg.message);
188                     }
189                     broadcast(r,e);
190                 }
191             }
192         } else if (msg.eventsToPush instanceof AtmosphereResourceEvent) {
193             AtmosphereResource r = (AtmosphereResource) msg.eventsToPush;
194             synchronized (r) {
195                 if (r instanceof AtmosphereResourceEventImpl) {
196                     e = ((AtmosphereResourceImpl)r).event();
197                     e.setMessage(msg.message);
198                 }
199                 broadcast(r,e);
200             }
201         } else if (msg.eventsToPush instanceof Set) {
202             Set<AtmosphereResource> sub =
203                     (Set<AtmosphereResource>) msg.eventsToPush;
204             for (AtmosphereResource r : sub) {
205                 synchronized (r) {
206                     if (r instanceof AtmosphereResourceEventImpl) {
207                         e = ((AtmosphereResourceImpl)r).event();
208                         e.setMessage(msg.message);
209                     }
210                     broadcast(r,e);
211                 }
212             }
213         }
214     }

As you see at line 195 and line 206 a cast i made to AtmosphereResourceEventImpl not to AtmosphereResourceImpl. This would also mean broadcasting to a single resource might not work as well.

Kind regards,

Cagatay

Jeanfrancois Arcand

Re: Possible bug in DefaultBroadcaster?

Reply Threaded

Salut,

fixed.

Never use a tool when refactoring as you ends up breaking too many
things :-(. Another reason why I need to focus on unit test next week in
between travel destination!

Thanks for your patience.

-- Jeanfrancois

Cagatay Civici wrote:

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Viktor Klang

Re: Possible bug in DefaultBroadcaster?

Reply Threaded

Dang, was just getting ready to merge this...

Thanks for the speed Jean-Francois!

On Sun, Nov 8, 2009 at 12:40 AM, Jeanfrancois Arcand <[hidden email]> wrote:

Salut,

fixed.

Never use a tool when refactoring as you ends up breaking too many things :-(. Another reason why I need to focus on unit test next week in between travel destination!

Thanks for your patience.

-- Jeanfrancois

Cagatay Civici wrote:

Hi,

I was tackling with a problem of broadcasting to a subset of AtmosphereResources, when using DefaultBroadcaster.broadcast(message) everything is fine but with DefaultBroadcaster.broadcast(message, Set set)
the onStateChange is called with a null AtmosphereResourceEvent.

I'm not a Atmosphere code guru but the push implementation in Broadcast could be the problem;

*protected* *void* push(Entry <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html> msg) {
181 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#181> AtmosphereResourceEventImpl <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/AtmosphereResourceEventImpl.html> e = *null*;
182 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#182> *if* (msg.eventsToPush == *null*) {
183 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#183> *for* (AtmosphereResource r : events) {
184 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#184> *synchronized* (r) {
185 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#185> *if* (r instanceof AtmosphereResourceImpl) {
186 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#186> e = ((AtmosphereResourceImpl)r).event();
187 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#187> e.setMessage(msg.message);
188 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#188> }
189 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#189> broadcast(r,e);
190 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#190> }
191 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#191> }
192 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#192> } *else* *if* (msg.eventsToPush instanceof AtmosphereResourceEvent) {
193 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#193> AtmosphereResource <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/AtmosphereResource.html> r = (AtmosphereResource) msg.eventsToPush;
194 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#194> *synchronized* (r) {
195 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#195> *if* (r instanceof AtmosphereResourceEventImpl) {
196 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#196> e = ((AtmosphereResourceImpl)r).event();
197 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#197> e.setMessage(msg.message);
198 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#198> }
199 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#199> broadcast(r,e);
200 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#200> }
201 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#201> } *else* *if* (msg.eventsToPush instanceof Set) {
202 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#202> Set<AtmosphereResource> sub =
203 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#203> (Set<AtmosphereResource>) msg.eventsToPush;
204 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#204> *for* (AtmosphereResource r : sub) {
205 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#205> *synchronized* (r) {
206 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#206> *if* (r instanceof AtmosphereResourceEventImpl) {
207 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#207> e = ((AtmosphereResourceImpl)r).event();
208 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#208> e.setMessage(msg.message);
209 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#209> }
210 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#210> broadcast(r,e);
211 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#211> }
212 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#212> }
213 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#213> }
214 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#214> }

As you see at line 195 and line 206 a cast i made to AtmosphereResourceEventImpl not to AtmosphereResourceImpl. This would also mean broadcasting to a single resource might not work as well.

Kind regards,

Cagatay

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

--
Viktor Klang
| "A complex system that works is invariably
| found to have evolved from a simple system
| that worked." - John Gall

Blog: klangism.blogspot.com
Twttr: twitter.com/viktorklang
Code: github.com/viktorklang

Cagatay Civici

Re: Possible bug in DefaultBroadcaster?

Reply Threaded

In reply to this post by Jeanfrancois Arcand

Thanks for the quick fix. I was about to create a patch :)

Testing with a local build, I can confirm fix works perfectly!

Cagatay

On Nov 7, 2009, at 11:40 PM, Jeanfrancois Arcand wrote:

> Salut,
>
> fixed.
>
> Never use a tool when refactoring as you ends up breaking too many
> things :-(. Another reason why I need to focus on unit test next
> week in between travel destination!
>
> Thanks for your patience.
>
> -- Jeanfrancois
>
> Cagatay Civici wrote:
>> Hi,
>> I was tackling with a problem of broadcasting to a subset of
>> AtmosphereResources, when using DefaultBroadcaster.broadcast
>> (message) everything is fine but with DefaultBroadcaster.broadcast
>> (message, Set set)
>> the onStateChange is called with a null AtmosphereResourceEvent.
>> I'm not a Atmosphere code guru but the push implementation in
>> Broadcast could be the problem;
>> *protected* *void* push(Entry <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html
>> > msg) {
>> 181 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#181
>> > AtmosphereResourceEventImpl <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/AtmosphereResourceEventImpl.html
>> > e = *null*;
>> 182 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#182
>> > *if* (msg.eventsToPush == *null*) {
>> 183 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#183
>> > *for* (AtmosphereResource r : events) {
>> 184 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#184
>> > *synchronized* (r) {
>> 185 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#185
>> > *if* (r instanceof AtmosphereResourceImpl) {
>> 186 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#186
>> > e = ((AtmosphereResourceImpl)r).event();
>> 187 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#187
>> > e.setMessage(msg.message);
>> 188 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#188
>> > }
>> 189 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#189
>> > broadcast(r,e);
>> 190 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#190
>> > }
>> 191 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#191
>> > }
>> 192 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#192
>> > } *else* *if* (msg.eventsToPush instanceof
>> AtmosphereResourceEvent) {
>> 193 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#193
>> > AtmosphereResource <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/AtmosphereResource.html
>> > r = (AtmosphereResource) msg.eventsToPush;
>> 194 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#194
>> > *synchronized* (r) {
>> 195 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#195
>> > *if* (r instanceof AtmosphereResourceEventImpl) {
>> 196 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#196
>> > e = ((AtmosphereResourceImpl)r).event();
>> 197 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#197
>> > e.setMessage(msg.message);
>> 198 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#198
>> > }
>> 199 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#199
>> > broadcast(r,e);
>> 200 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#200
>> > }
>> 201 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#201
>> > } *else* *if* (msg.eventsToPush instanceof Set) {
>> 202 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#202
>> > Set<AtmosphereResource> sub =
>> 203 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#203
>> > (Set<AtmosphereResource>) msg.eventsToPush;
>> 204 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#204
>> > *for* (AtmosphereResource r : sub) {
>> 205 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#205
>> > *synchronized* (r) {
>> 206 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#206
>> > *if* (r instanceof
>> AtmosphereResourceEventImpl) {
>> 207 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#207
>> > e = ((AtmosphereResourceImpl)r).event();
>> 208 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#208
>> > e.setMessage(msg.message);
>> 209 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#209
>> > }
>> 210 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#210
>> > broadcast(r,e);
>> 211 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#211
>> > }
>> 212 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#212
>> > }
>> 213 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#213
>> > }
>> 214 <https://atmosphere.dev.java.net/nonav/xref/org/atmosphere/cpr/DefaultBroadcaster.html#214
>> > }
>> As you see at line 195 and line 206 a cast i made to
>> AtmosphereResourceEventImpl not to AtmosphereResourceImpl. This
>> would also mean broadcasting to a single resource might not work as
>> well.
>> Kind regards,
>> Cagatay
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]