[MGNLRSSAGG-195] ConcurrentModificationException exception after setting "Use different settings for this feed" Created: 24/Feb/15  Updated: 01/Jul/15  Resolved: 04/Mar/15

Status: Closed
Project: Magnolia RSS Aggregator Module
Component/s: None
Affects Version/s: 2.3.3
Fix Version/s: 2.3.4

Type: Bug Priority: Blocker
Reporter: Evzen Fochr Assignee: Evzen Fochr
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relation
is related to MGNLRSSAGG-166 Re-add again an easy way to schedule ... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

Error java.util.ConcurrentModificationException possibly caused by two jobs(but same feed) concurently try to execute at same time. It start after i set "Use different settings for this feed".



 Comments   
Comment by Jan Haderka [ 27/Feb/15 ]
  • jobs are using fetchers not the other way around, so on shutdown you need to first stop execution of the fetchers and then shutdown jobs, not the other way around.
  • removal of fetcher from the job, means that all the jobs will be forced to use just one fetcher, while before it was possible to use different fetchers for different jobs (e.g. fast one for critical jobs, simple one for non critical jobs)
  • move of executor service to the module class itself makes it tied to the fast fetcher, while this is hardly the only available fetcher. There are others that don't need extra threads so you should keep that piece of fetcher functionality in the fetcher instead of pushing it up.
Comment by Jan Haderka [ 27/Feb/15 ]
  • RSSAggregatorConfigurationPresenter.onManualImport
    I'm afraid that invoking LaunchRSS.. command there w/o any configuration will do just nothing
  • RSSAggregator now looks much better ... and it is much more clean that it should be absolutely separated from the FastRSSFetcher ... let's try to come up with the way to get rid of it
  • please rename SetPropertyFetcherClass to SetFetcherClassPropertyTask
Comment by Jan Haderka [ 04/Mar/15 ]
  • RssAggregator.mapRSSJob()
    • you should use sql2 query syntax rather than sql when possible
    • there should be no need to catch generic Exception no?
    • there should be try/catch inside of the while loop so if one job fails to be scheduled, others are not affected by this change
    • name of that method sucks, it doesn't do any mapping does it?
  • similarly for removal of jobs there's convoluted sequence of calls from removeRSSJobs() to removeRSSJob() to removeJob(). Do we really want all that fragmentation there?
  • why is FeedSyndicationServlet deprecated? What is it replaced with? This servlet is responsible for producing feeds from either aggregates or magnolia content or just anything. If it is now superseeded by something, deprecation method should mention it and you would need related updates in STK, categorization and other modules that use this functionality to produce their own feeds.

But in general, yeah, it's moving in the right direction

Comment by Evzen Fochr [ 04/Mar/15 ]

sql to sql2 - done
no generic Exception - done
try/catch inside of the while loop - done
methods renamed - done
removeJob() deprecated without replacement.
FeedSyndicationServlet added back, created new ticket for change to new parent class (old one is deprecated)

Comment by Jan Haderka [ 05/Mar/15 ]
jobs.put(job.getName(), job);
addJobToScheduler(job);

but the add method has some condition inside which might result in job not really being added to scheduler ... is it still correct to add it to the list of jobs?

/**
* @deprecated since 2.3.4, use {@link #RSSAggregator(SchedulerModule scheduler)} instead.
*/
@Deprecated
public RSSAggregator() {
}

good job depracting that ctor, but you still need to initialize scheduler inside it otherwise someone using it would end up with NPE. Just because it's deprecated doesn't mean it should not be functional.

Comment by Evzen Fochr [ 05/Mar/15 ]

jobs.put(job.getName(), job); is needed because job is called by actions "Import feed now" and "Import all feeds now" even if job is not scheduled for automatic import.

Generated at Mon Feb 12 07:06:30 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.