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 overeager subscription cleanup #712

Merged
merged 1 commit into from Jan 24, 2017

Conversation

Projects
None yet
7 participants
@hartez
Member

hartez commented Jan 24, 2017

Description of Change

Fixes incorrect subscription cleanup logic which removed subscriptions which were not eligible for removal. Adds unit test to catch the problem in the future.

Bugs Fixed

API Changes

  • None

Behavioral Changes

  • None

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
@@ -205,7 +205,7 @@ static void InnerUnsubscribe(string message, Type senderType, Type argType, obje
var key = new Sender(message, senderType, argType);
if (!s_subscriptions.ContainsKey(key))
return;
s_subscriptions[key].RemoveAll(sub => !sub.CanBeRemoved() || sub.Subscriber.Target == subscriber);

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Jan 24, 2017

Member

🤦‍♂️

@StephaneDelcroix

StephaneDelcroix Jan 24, 2017

Member

🤦‍♂️

This comment has been minimized.

@hartez

hartez Jan 24, 2017

Member

Yeah.

@hartez

hartez Jan 24, 2017

Member

Yeah.

This comment has been minimized.

@bentmar

bentmar Jan 24, 2017

Contributor

👍 Thanks for the quick fix "!" :)

@bentmar

bentmar Jan 24, 2017

Contributor

👍 Thanks for the quick fix "!" :)

@rmarinho rmarinho merged commit ebb24b7 into master Jan 24, 2017

2 of 3 checks passed

iOS9-UITests-C8 Started TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3705, ignored: 10
Details

rmarinho added a commit that referenced this pull request Jan 30, 2017

@EmmanVazz

This comment has been minimized.

Show comment
Hide comment
@EmmanVazz

EmmanVazz Feb 3, 2017

I am getting a crash that seems to be the result of these changes.

Exception:
System.NullReferenceException: Object reference not set to an instance of an object
StackTrace:
Xamarin.Forms.MessagingCenter.<>c__DisplayClass14_0.b__0(MessagingCenter.Subscription sub)
System.Collections.Generic.List.RemoveAll(Predicate match)list.cs:866
Xamarin.Forms.MessagingCenter.InnerUnsubscribe(string message, Type senderType, Type argType, object subscriber)
Xamarin.Forms.MessagingCenter.Unsubscribe<TSender, TArgs>(object subscriber, string message)
AppTemplate2.ViewModels.NodeRetrievalModel.UnsubcribeFromMessages()NodeRetrievalModel.cs:104
AppTemplate2.Views.Text.Finalize()Text.xaml.cs:22

Any thoughts from the stack trace?

I am getting a crash that seems to be the result of these changes.

Exception:
System.NullReferenceException: Object reference not set to an instance of an object
StackTrace:
Xamarin.Forms.MessagingCenter.<>c__DisplayClass14_0.b__0(MessagingCenter.Subscription sub)
System.Collections.Generic.List.RemoveAll(Predicate match)list.cs:866
Xamarin.Forms.MessagingCenter.InnerUnsubscribe(string message, Type senderType, Type argType, object subscriber)
Xamarin.Forms.MessagingCenter.Unsubscribe<TSender, TArgs>(object subscriber, string message)
AppTemplate2.ViewModels.NodeRetrievalModel.UnsubcribeFromMessages()NodeRetrievalModel.cs:104
AppTemplate2.Views.Text.Finalize()Text.xaml.cs:22

Any thoughts from the stack trace?

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Feb 3, 2017

Member

@EmmanVazz Without seeing the code in NodeRetrievalModel.UnsubcribeFromMessages() it's hard to say. I suggest opening an issue in Bugzilla and posting your stacktrace (and ideally a small repro project).

Member

hartez commented Feb 3, 2017

@EmmanVazz Without seeing the code in NodeRetrievalModel.UnsubcribeFromMessages() it's hard to say. I suggest opening an issue in Bugzilla and posting your stacktrace (and ideally a small repro project).

@hartez hartez deleted the fix-bugzilla51703 branch May 16, 2017

@samhouts samhouts added this to the 2.3.4 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment