cxf-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Amichai Rothman (JIRA)" <j...@apache.org>
Subject [jira] [Created] (DOSGI-161) services sometimes don't get exported
Date Sun, 24 Mar 2013 11:37:15 GMT
Amichai Rothman created DOSGI-161:
-------------------------------------

             Summary: services sometimes don't get exported
                 Key: DOSGI-161
                 URL: https://issues.apache.org/jira/browse/DOSGI-161
             Project: CXF Distributed OSGi
          Issue Type: Bug
          Components: DSW
    Affects Versions: 1.4
         Environment: Oracle JDK 1.7.0_17, Karaf 2.3.1, DOSGi 1.4.0 (installed from Karaf
feature)
            Reporter: Amichai Rothman


When starting up a fresh Karaf instance repeatedly, a service that should be exported sometimes
does not in fact get exported. The issue stems from some bugs in the TopologyManagerExport
class, the main ones being wrong assumptions about the state of the Tracker within its callback
method (during the callback its internal state is not yet updated, but the code assumes it
is), unconditional overwriting of data structures (which may already exist) and synchronization
issues.

During my investigations I came across various other small issues and enhancements in the
class, so the provided patch includes the final result I arrived with including all changes.
I've split it up into several consecutive patches in order to make the review easier. These
are the changes:

Patch #1 - cleanup (no semantic changes):
- fixed typos in docs and logs.
- fixed formatting to be more consistent and readable.
- added debug logs to RemoteServiceAdminLifeCycleListener.added/removed callbacks (this proved
the serious tracker bug below, but is also useful for general debugging).
- simplified shouldExportService returned boolean expression.
- removed removeExportRegistration, removeExportReference and remoteAdminEvent methods, all
of which are both unused and have a no-op implementation.

Patch #2 (improved iterations):
- replaced iterations on map keys followed by a get of the corresponding value with an iteration
on Map.Entry (prevents unnecessary lookups).
- replaced map containsKey checks followed by get with a get followed by null check (prevents
unnecessary lookups, and theoretically race conditions too).
- fixed remoteServiceAdmin.exportService return value handling - its docs say the result can
never be null, but the code assumes it might be. This is now replaced with an isEmpty() check
instead, I hope there are no other consequences. The reason this is bundled with the iterations
fixes is since it shows explicitly that map values cannot in fact be null, which proves the
iteration fixes to be correct (i.e. if Map.get returns null, the key does not exist, so this
is equivalent to calling Map.containsKey).

Patch #3 (tracker and sync issues):
- fixed tracker misuse: when tracker callback (added) is called, the tracker itself does not
yet reflect the new state, so accessing services (or size) on the tracker from within the
callback will fail to include the new service. Although the actual service export triggered
by the callback is performed on a separate thread, this still leaves a race condition between
the two which sometimes causes the export to fail (with "No RemoteServiceAdmin available!"
log). To fix this, the newly added RSA is passed down the method chain to the doExportService
method explicitly. doExportService then uses the reference if it is given, or if null (as
when called from elsewhere), uses the tracker to get the service list as it did in the past.
- fixed exportService always overwriting a service's entry in exportedServices with a new
empty data structure - now it only does so if it does not already exist.
- fixed exportedServices inner maps to be created as synchronized maps - previously doExportService
wrapped it in a synchronized map on each invocation, which results effectively in no synchronization,
since the synchronized wrapper map is local only so each thread accessing the map uses a different
lock. Also fixed other accesses of the inner maps to be synchronized (Collections.synchronizedMap
requires external synchronization on it when being iterated, as specified in the docs).
- fixed exports null check in doExportService (in the old code, Collections.synchronizedMap
never returns null; In the new code it makes more sense in case of concurrent removal.)
- removed redundant remoteServiceAdminTracker null check in doExportService (it is already
used in the constructor, so can't be null at this point).


Please review the changes to make sure nothing was missed (a cluster of issues in the same
code section usually implies additional lurking bugs...), and feel free to drop any non-critical
changes that you don't like - I find them all beneficial, but it can be a matter of taste.


--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message