[WFCORE-1235] : Multiple triggers of each activator service when there are multiple modules with different service activator files in an ear. #1317
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Retest this please. |
|
retest this please Failure was unrelated. |
|
Could someone verify this patch? Thanks. |
panossot
force-pushed
the
WFCORE-1235
branch
from
aa42ca2
to
c9c36ca
Compare
|
retest this please Failure was unrelated. |
1 similar comment
|
retest this please Failure was unrelated. |
|
Failure is unrelated |
|
Retest this please. |
|
retest this please Failure was unrelated. |
|
I do not think this PR fixes the underlying problem. If two deployments have visibility to each others services directories then it is still possible for the ServiceActivator to be run twice. |
|
@stuartwdouglas Could you review again? |
|
Retest this please. When run locally tests pass. |
|
SingletonServiceTestCase fails. I will have to rethink it. |
panossot
force-pushed
the
WFCORE-1235
branch
3 times, most recently
from
be7e3d7
to
8568f57
Compare
|
@stuartwdouglas Could you review again? (Solution without VFS) |
|
Retest this please. Local tests pass. |
…e are multiple modules with different service activator files in an ear..
|
Full integration - Windows Build 1757 outcome was FAILURE using a merge of c470c6d Failed tests |
|
Retest this please. Irrelevant failures. |
|
Core - Full Integration Build 3316 outcome was FAILURE using a merge of c470c6d Failed tests |
|
Retest this please. Irrelevant failures. |
|
@dmlloyd I'd like to get this one approved or rejected. Here's my read on what it is doing:
a) It loops through the name of ServiceActivator impls recorded in the current deployment's SERVICES attachment i) if the SA impl in outer loop 2) isn't included in the list 2a), it won't get activated. I don't know if this is a feature, a bug or just superfluous. IOW I don't know what it means that the impl is found by ServiceLoader but wasn't listed in the SERVICES attachment. My guess is it's a feature; we don't want to activate SA impls just because they are visible via ServiceLoader; we only want them if ServiceLoaderProcessor decided they were relevant. b) else if the name of the SA impl is included in the list of subdeployment SA impls created in 1) above, the SA is not activated. Otherwise it is. The effect of 2b) is that if the same SA impl is declared in both the parent level and the subdeployment level, it doesn't get activated in the parent level. Here I don't know if this behavior is a feature or necessary to get standard behavior to work. IOW is it required for some reason that the user has a META-INF/services file for the SA impl in both the parent and the subdeployment? If it isn't required, do we want to support being "helpful" here, in the way 2b) is? (I vote no.) Apologies if some of this has been discussed in past email threads or issue comments. But they haven't resulted in this PR being approved or rejected, and it's time to do one or the other. |
|
Your assessment in 2a) seems correct to me. The problem stems from subdeployments which all "see" one another, which in turn cases them to have the same list of activators, which in turn causes each one to be started one time for each subdeployment. For your 2b) question, if a user has an SA in both parent and subdeployment, then it doesn't really make a lot of sense no matter what we do. Activating the service on the subdeployment but not the parent/other deployments seems like OK behavior to me, but you could easily argue the opposite. I wouldn't call it a "feature" in any event. |
|
@dmlloyd re 2b), yes, "feature" is too strong a word. Basically I just mean being nice and working around something that is actually an incorrect deployment. It's friendly to do but similar to a full on feature it generates an obligation to continue to do that, which limits future flexibility. I don't feel strongly about this though. |
|
Meh, the alternative is to detect this and throw a special exception, which is added work. Ignoring it and waiting for an DuplicateServiceException with no explanation why there's a dup is poor. So, I'll merge it. |
bstansberry
added
the
ready-for-merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[WFCORE-1235] : https://issues.jboss.org/browse/WFCORE-1235