Welcome!

Welcome to the official BlackBerry Support Community Forums.

This is your resource to discuss support topics with your peers, and learn from each other.

inside custom component

Java Development

Reply
Developer
peter_strange
Posts: 19,609
Registered: ‎07-14-2008
My Device: Not Specified
Accepted Solution

Unnecessary use of synchronize in sample code in Dev Guide

[ Edited ]

I ask this question with trepidation, I'm sure someone out there is going to be able to explain this and then I will feel really silly asking.  But here goes....

 

The Using an HTTP connection code sample in the Developer Guide (I've checked both 4.3 and 4.5 and it is the same in both these levels), use synchronized in two places that I don't understand.

 

1) getUrl() - Code follows

/**
* Retrieve the URL. The synchronized keyword makes sure that only one
* thread at a time can call this method on a ConnectionThread object.
*/
public synchronized String getUrl() {
    return _theUrl;
}

 

I understand the comment, and occasionally it is useful to force methods to be synchronized.  But not this method in isolation.  if anything, there is more of case for using this mechanism for the fetch method in this code.

 

2) Around the fetch block - Code follows:

/* Ensure that fetch requests are not missed
* while received data is processed.
*/
synchronized(this) {
    // Open the connection

 

When I see a block synchronization like this, I assume that it is a shared bit of code that can be executed by multiple threads, which this clearly isn't.  So why is it synchronized? 

 

I actually think the 'design' demonstrated by this sample may be flawed.  I suspect it would be better to have a new Thread for each connection, or have a (synchronized) queue of URLs to be processed.

 

As evidenced by another thread on this forum, developers do take these samples as gospel and try to use them.  So if this one is wrong, perhaps we could fix it.  But perhaps it just me being a complete dolt.  Wouldn't be the first time.....  And won't be the last!

Message Edited by peter_strange on 09-11-2008 08:45 PM
Please use plain text.
Developer
marchywka
Posts: 1,415
Registered: ‎07-30-2008
My Device: Not Specified

Re: Unnecessary use of synchronize in sample code in Dev Guide

I guess looking at the code you posted on the other thread I would make several comments:

1) See sun.com for a lot of good intro material,

2) Use volatile on shared variables, and you can use sync blocks to force one thread to publicize writes and make operations atomic,

no paricular piece of code need be shared by two threads

3) Keep sync areas small and only when needed- I think early sun libraries just put sync on all methods and

it was slow and prone to deadlock for no reason, I don't think I ever synchronize methods,

4) Be aware of issues with blocking UI threads and UI components,

5) Close real connections as soon as possible or have a mechanism worked out to reuse them,

6)Object re-use or pooling is good and should be encouraged

 

I guess maybe that synchronized get  is just supposed to wait for the run() method to release the lock but there is

probably some other approach.

Please use plain text.
Developer
peter_strange
Posts: 19,609
Registered: ‎07-14-2008
My Device: Not Specified

Re: Unnecessary use of synchronize in sample code in Dev Guide

All good points marchywka, relating to where and when you should use synchronize and other good coding practices.  However, the point I'm trying to make is simply that the use of synchronize in the sample does not appear, to me at least, to be a good example - the reverse in fact.  I'm not trying to create a primer, I'm asking if other people agree - if so perhaps we can ask Rim to correct the sample. 

 

Regarding your specific point abut the getUrl, as far as I can see in this case, there is no good reason to block one thread from finding out the URL while another thread is also finding out.  I don't think getUrl will wait for the block in run method, but if it did, that would make the getUrl a very dangerous method - imagine if someone called it while holding the Event Lock, and was then effectively blocked until the response came back!

Please use plain text.
Retired
bzubert
Posts: 86
Registered: ‎07-11-2008
My Device: Not Specified

Re: Unnecessary use of synchronize in sample code in Dev Guide

Peter,

 

HTTPDemo.fetchPage() is called from _fetchMenuItem and _fetchTTPSPage, which in turn calls ConnectionThread.fetch(), which contains one of our synchronization blocks. In this case, we're defending against the simultaneous modification of _start and _theUrl, which can be modified in both the ConnectionThread itself and by the event thread. The next big synchronized block, inside of the run() method, makes sure that we are executing our connection to completion before updating the value of _start back to false, thus ensuring that if the user is pounding on those menu items, nothing is missed.

Brian Zubert
Technical Partnership Manager
Research In Motion
Please use plain text.
Developer
peter_strange
Posts: 19,609
Registered: ‎07-14-2008
My Device: Not Specified

Re: Unnecessary use of synchronized in sample code in Dev Guide

[ Edited ]

Thanks for your comments Brian.

 

Note that my query was directed at the sample (HttpFetch) provided in the Developer Guide, not the sample (HTTPDemo) provided with the JDE samples.  Would you be able to review my comments with respect to HttpFetch in the manual?

 

I've now looked seriously at the HTTPDemo sample in the 4.5 JDE, in light of your comments.  As with my initial trepidation regarding the manual, I'm aware that I could be talking tosh here - sometimes the intricacies of multi-threaded systems are difficult to understand.  I'm prepared to be shot down in flames!  But I'm not convinced about the use of synchronized in the JDE supplied sample either.

 

HTTPDemo.fetchPage() is called from the two places you mentioned, and I note that _fetchMenuItem.run() is also invoked via a keyChar() method.  In all these cases, the invocation is done directly from the Event Thread, which is a single thread.  This means the user can not be issuing these at the same time, so the synchronize does not achieve anything.  And worse, if it did actually block (in other words, there was a page being fetched), it would block the Event thread until the page was fetched!.   I think it is the isStarted() checks that ensure that nothing is missed (or more accurately, a message is displayed when something would be missed).  It seems to me that the synchronized on the big block within ConnectionThread and the synchronized in ConnectionThread.fetch() could both be removed with no impact on the processing.

 

I also note that the getUrl() method is synchronized in HTTPDemo.  Is there any value in that?

 

I'm sorry if it appears so, but I'm not trying to pick holes for the sake of it.  People copy sample code and assume it shows good practice. 

Message Edited by peter_strange on 09-12-2008 08:45 PM
Please use plain text.
Developer
marchywka
Posts: 1,415
Registered: ‎07-30-2008
My Device: Not Specified

Re: Unnecessary use of synchronized in sample code in Dev Guide

I think I finally decided not a post a prior set of comments but just a few thoughts.

First, you don't afaik want to use a monitor wait as a designed barrier and you certainly

don't want the UI thread competing with a network connection for a lock. For read-modify-writes,

for various signalling approaches, publicizing results to other thread,  sync blocks are great. If you want make another thread wait by

design, and not just breifly due to accidental time coincidences, you can use a wait/notify

and catch interrupted exceptions (IIRC) if someone else needs to abort. I may be wrong but I think

you are stuck waiting for a monitor at sync entry althought IIRC you could catch a ThreadDeathError if you try to kill it.

There is often a tendency to just throw sync's in to be safe etc but that just invites starvation and

ultimately dead lock since you usually lose track of what you are locking with ( code becomes

as awkward as my sentences,LOL).

 

Please use plain text.
Retired
bzubert
Posts: 86
Registered: ‎07-11-2008
My Device: Not Specified

Re: Unnecessary use of synchronized in sample code in Dev Guide

It looks like this sample was designed with a basic defense to the following scenario (if no synchronization):

 

Event thread requests _start = true.

Connection thread begins the conneciton.

Event thread again requests _start = true.

Connection thread completes the connection, sets _start = false. 

 

Net result: the second event thread request is missed. 

 

That being said, you guys have it right:  this design is poor at best.  In the above scenario, the second event thread request is blocked while the connection thread completes... and as everyone well knows at this point, blocking the event thread usually leads to a world of hurt.  There are certainly better approaches that could be used here and likely the easiest solution to this problem is just to have better control flow, such as not allowing a new connection request while one is already under way.  The synchronization certainly adds undue complexity to this problem and distracts from what the sample is attempting to demonstrate: a simple HTTP connection. 

 

For both this sample and the fetch, I'll recommend to the authors that both be re-examined and a structure be set up such that these samples remain as simple as possible.  Thanks to both Peter and marchywka for their suggestions.  Feedback like yours is how we identify areas for improvement. 

Brian Zubert
Technical Partnership Manager
Research In Motion
Please use plain text.
Developer
marchywka
Posts: 1,415
Registered: ‎07-30-2008
My Device: Not Specified

Re: Unnecessary use of synchronized in sample code in Dev Guide

Just as a matter of robustness, you don't want to have locks when you depend on an external entity to do something.

(wait of course is in a sync block but it releases the lock).

First, there is simply no way to know when they will come back. Second, you never know what other locks they

will pickup. The entities of course vary in risk levels but neither the network or user should be relied upon to

do the right thing if you want the code to work :smileyhappy: Generally, assignments and math  are pretty safe but I don't like counting

on much more than that to go right.

 

Also, as noted in the one example, when you set two things in one thread, there is no guarantee that another thread

will observe the assignments in order or even at all unless they are volatile,

 

http://java.sun.com/docs/books/jls/second_edition/html/memory.doc.html

 

 

Please use plain text.