Can I modify a list concurrently with multiple different methods ?

[Bund De]

Ok firstly, I know my code is an absolute mess, sorry bout that, but I'm just trying to get this thing done so I don't feel like I've wasted time.
So, I'm taking an online course at Udacity to build a basic search engine, the class is in Python but I'm doing it in Java for the extra challenge.

The problem is, I'm trying to simultaneously add links to a list, and loop through it to get the next links until all the links are extracted, as you can see in the crawl_web() and get_all_links() methods.

I've looked through the web and found something about Synchronization, but I probably did it wrong and it's not working.

So, can this be done ? Is there a better way to do it ?

public class WebCrawler{

static List links;
static URL seed;
       
	public static void main(String[] args) throws IOException {
		links = new ArrayList();
	     seed = new URL("http://www.udacity.com/cs101x/crawling.html");
		
		links.add(seed.toString());
		crawl_web();
		System.out.println(links);
	}
	
	public static void crawl_web() throws IOException{
		synchronized (links) {
                //here lies the problem
			for(Object x:links){//goes through the list to get the next links to crawl
				seed = new URL(x.toString());
                //extracts all the links from a page and stores it in "links"
				get_all_links(0,getContentResult(seed));
			}                                           
		}
	}
	
	public static void get_all_links(int end_index, String str) throws IOException{
		while(end_index!=-1){
			  end_index=get_links(end_index, str);
		}
	}
	
	public static int get_links(int start, String str){
		int index=str.indexOf("http://", start);
		
		if(index==-1) return index;
		
		int end_index = str.indexOf('"',index);
		String   nova = str.substring(index,end_index);
		
		synchronized (links) {
      //the "notIn" method tests if the link is already stored, to avoid an infinite loop
			if(notIn(nova)){
				links.add(nova);
			}
		}	        
		return end_index;
	}
	
	public static boolean notIn(String str){
		String compara;
		synchronized (links) {
			for(Object x:links){
				compara=x.toString();
				if(str.equals(compara)) return false;
			}
		}
		return true;
	}

[Steve Luke]

The ConcurrentModificationException you are getting is not related to threaded concurrency (despite the name), so synchronization will not help you. In this case, the problem comes from the fact that you are trying to manipulate the list directly while iterating over it using an iterator (the enhanced for loop), which you can't do. Instead, you should look at the API for java.util.List (<- link) which provides a solution.

[Bund De]

Is the answer "CopyOnWriteArrayList" ? Let me try that one.

[Stephan van Hulst]

No. The answer is to give your methods clear responsibilities with pre- and postconditions to go with them.

Why is a method named get_links() adding something to your links? This is a big design flaw. Don't patch your code to solve problems, write clear code to start with.

[Bund De]

I don't understand, the name is a problem ? get_links, add_to_links, isnt that the same thing ?

[Steve Luke]

No, the name isn't the problem, Stephen is saying the design is.

Regardless of that, your problem (let me repeat this) is not concurrency, threading, or anything like that. Please stay away from threading and concurrency at the moment because you clearly don't understand it.

The fix I mentioned in the API is to use a ListIterator: which allows you to modify a list as you iterate over it. But you can only access, add, or remove to/from the list through that ListIterator - not through the List itself. That means a hefty redesign (which you need to do anyway).

You can do that, or you can index manually (using an integer counter), or you could us a Queue (where you use a while((link = queue.poll())!=null) loop to get the links).

[Bund De]

Thanks I'll look into that