Skip to content

Revert "Fix a crash on Android"#15

Open
xzysolitaire wants to merge 30 commits into
dragons3d_masterfrom
DRG-42245
Open

Revert "Fix a crash on Android"#15
xzysolitaire wants to merge 30 commits into
dragons3d_masterfrom
DRG-42245

Conversation

@xzysolitaire

Copy link
Copy Markdown

This reverts commit c179213.

mjk3979 and others added 28 commits July 27, 2015 18:20
This is so we can set their timeout
This fixes the keep-alive issue for an unkown reason.
There is an Apple Technical Support Request for this issue, follow up number 629273694
Return description in case of unknown type
Added a hopeful workaround to the crash from copying serviceData
This is unnecessary and may cause a crash in thread unsafe code
Fixed service data crash due to unsafe threads
Conflicts:
	.gitignore
@mjk3979

mjk3979 commented Aug 24, 2016

Copy link
Copy Markdown

We should actually just remove my change that caused this too, it's unneeded now. I'm pretty sure Aaron had a good reason for removing this from Android.

@xzysolitaire

Copy link
Copy Markdown
Author

If we remove your commit, the problem will still happen on android. So maybe we need to take the method of disabling subscription requests upon going to background.
I have reviewed the PNSubscriber.m already but not got a good idea of how to do this properly. Maybe need more time to think about it.

#pragma mark - Internal
- (void)didEnterBackground {
[self.client.heartbeatManager stopHeartbeatIfPossible];
[self stopRetryTimer];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this start when the game reenters the foreground?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not quite sure about how it works but when I set a breakpoint at startHeartbeat it get called when the game comes back. The stack trace shows this is triggered by some callback.
QA has verified the functionality works fine. Yeah I don't know if there is a better solution to this problem.

@mjk3979

mjk3979 commented Sep 8, 2016

Copy link
Copy Markdown

Why are we making this change? Is there a ticket?

#import "PNHelpers.h"

#ifdef PGDROID
#import <UIKit/UIKit.h>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this import?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XCode complains the UIApplication notification is not defined.
I checked in our code base and saw Orlando did something like this elsewhere.

@mjk3979

mjk3979 commented Sep 8, 2016

Copy link
Copy Markdown

If we are doing this to stop PubNub traffic in the background, then I think a safer, better, and cleaner solution would be to tear down the PubNub instance from inside PERealTimeCommunicationManager when we go to background, and then start it back up when we reenter the foreground.

@xzysolitaire

Copy link
Copy Markdown
Author

yes ticket number is DRG-42245.

The solution you mentioned would stop all the background pubnub traffic so I really have no idea if it would cause any other problems. Also performance might be another concern? Client would have to join channels and receive messages again every time the player backgrounds the game. And what David mentioned in the original ticket is only concerning subscribe request.

@mjk3979

mjk3979 commented Sep 19, 2016

Copy link
Copy Markdown

We already fetch history when we return from background (at least I think). And rejoining the channel group is only one request. There would be a minor performance hit, but I think it would be insignificant. I think removing the PubNub instance would be a cleaner and safer solution to this problem. Let me know if you still disagree and we can discuss it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants