New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix saga and timeout ATs #4176
Fix saga and timeout ATs #4176
Conversation
|
||
class SubscriptionBehavior<TContext> : IBehavior<IIncomingPhysicalMessageContext, IIncomingPhysicalMessageContext> where TContext : ScenarioContext | ||
class SubscriptionBehavior<TContext> : Behavior<IIncomingPhysicalMessageContext> where TContext : ScenarioContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try not to use the base class. Everything internal even the ATTs should use IBehavior, that class is now more of a customer focused convenience class
Retargeted against develop |
var retries = 0; | ||
var succeeded = false; | ||
Exception lastError = null; | ||
while (retries < maxRetries && !succeeded) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this retrying all messages? (shouldn't we only do this if the current message is a subscribe/unsubscribe message)
@andreasohlund @danielmarbach I've fixed the problems you found and also updated the external timeout manage test removing the dependency on timing. |
@SzymonPobiega did you forget to save the proj? Build seems to fail |
|
||
class SubscriptionBehavior<TContext> : IBehavior<IIncomingPhysicalMessageContext, IIncomingPhysicalMessageContext> where TContext : ScenarioContext | ||
class SubscriptionBehavior<TContext> : IBehavior<IIncomingPhysicalMessageContext> where TContext : ScenarioContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be IBehavior<IIncomingPhysicalMessageContext, IIncomingPhysicalMessageContext>
@danielmarbach nope. But I missed a compiler warning |
OK, not should be good. I used not that |
@SzymonPobiega for ATTs it does not matter that much. For production code internally it does because the behavior base class creates a new closure on each invocation call. We weren't able to refactor it without introducing breaking changes for 6.0 |
@andreasohlund LGTM |
return new SubscriptionBehavior<TContext>(action, context, MessageIntentEnum.Subscribe); | ||
})); | ||
return new SubscriptionBehavior<TContext>(action, context, builder.Build<CriticalError>(), MessageIntentEnum.Subscribe); | ||
}, DependencyLifecycle.InstancePerCall)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you add this? Afaik behaviors are cached anyway, so instance per call will not result in what you would expect when defining it?
what's the reason behind the retry logic? In what cases is this needed? rest looks good to me. |
@timbussmann persisters assume race conditions in subscription store are resolved via FLR. I know this is a bad assumption to begin with but we have to live with it till we get rid of these persisters entirely. |
RavenDB is already doing this should we raise issues for both NH and ASP to do the same? (not blocking this PR) |
I think this makes sense, otherwise this can hit anyone with disabled retries, so I'd say it's not a testing framework concern. I'm good with keeping it in case there are really tests encountering that race condition, but unless there is a real appearance I'd not pull that in and focus on the the subscription storages. |
@SzymonPobiega given that ravendb is good, where did you spot this? (inmemory? nh?) |
@andreasohlund NHibernate |
Should we consider add the inmem retries there instead of here? |
This would assume every persistence has this kind of feature. Should we have this assumption? I am fine with that. We just need to agree on this. |
I'd say yes, if there is potential race conditions the persister should try to mitigate that without relying on retries to be enabled in the endpoint? |
This discussion is similar to the discussion we had about ASB relying on retries when the broker connection goes boom. Should we hash this out? |
So should we fix NHibernate or merge this? Or merge this, fix NH later, remove this? |
I'd say fix NHibernate and remove this |
@Particular/nhibernate-persistence-maintainers thoughts? |
I'm not sure: @SzymonPobiega Will implementing the in-memory retries in subscription store alleviate the need for "wait until subscriber endpoint starts" in these ATTs? At a minimum we need to carve out the fix to #4178 out of this PR before closing it. I'll be happy to do this. |
(marking as wip to prevent it from merging at this point) |
@SzymonPobiega @MarcinHoppe any updates on this? |
@timbussmann We have an issue and a PR in NHibernate to address. I chatted with @Scooletz and ASP does not suffer from this issue (similar to Raven). /cc @SzymonPobiega |
sounds like we can close this issue and handle this on the NH repo then? |
I think it is still valid to wait till endpoints are started before subscribing. What do you think? Worth a separate PR? |
👍 to what @SzymonPobiega said. There's also issue with the external timeout manager test. I will file a separate PR to fix this single test. |
@@ -18,7 +18,7 @@ public Task Both_base_and_specific_events_should_be_delivered() | |||
await session.Publish(new SpecificEvent()); | |||
await session.Publish<IBaseEvent>(); | |||
})) | |||
.WithEndpoint<GeneralSubscriber>(b => b.When(async (session, c) => await session.Subscribe<IBaseEvent>())) | |||
.WithEndpoint<GeneralSubscriber>(b => b.When(c => c.EndpointsStarted, async (session, c) => await session.Subscribe<IBaseEvent>())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this when condition is necessary, as the ATT framework will execute whens already after endpoints have been started. See ScenarioRunner line 193.
bbb8e59
to
7b3ba94
Compare
@timbussmann I removed all the unnecessar changes and left only two:
|
LGTM |
@SzymonPobiega I moved the comment bullet points from here into the PR description is there anything else we need to update? |
Resolves #4178