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 #327: SendWorker now works in the separate process service. #329

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@romansl
Contributor

romansl commented Nov 23, 2015

This is a basic implementation.

Possible improvements:

  • Show notification while send task in progress (prevents from rare system-initiated process kill).
  • Merge SendService and SendWorker.
  • Self-stop SendService after delay.
  • Subscribe to "Internet availabale" broadcast and start SendService.
@william-ferguson-au

This comment has been minimized.

Show comment
Hide comment
@william-ferguson-au

william-ferguson-au Nov 25, 2015

Member

Will wait until 4.70 is released before merging this.

Member

william-ferguson-au commented Nov 25, 2015

Will wait until 4.70 is released before merging this.

@xmenxwk

This comment has been minimized.

Show comment
Hide comment
@xmenxwk

xmenxwk Dec 2, 2015

Very good but showing notification is something I would avoid.

xmenxwk commented Dec 2, 2015

Very good but showing notification is something I would avoid.

@andryr

This comment has been minimized.

Show comment
Hide comment
@andryr

andryr Dec 23, 2015

If a crash occurs on startup (eg in Application.onCreate()) it will enter in a infinite loop because of the Service.

andryr commented Dec 23, 2015

If a crash occurs on startup (eg in Application.onCreate()) it will enter in a infinite loop because of the Service.

@william-ferguson-au

This comment has been minimized.

Show comment
Hide comment
@william-ferguson-au

william-ferguson-au Dec 24, 2015

Member

@andryr how does having the service cause an infinite loop?

Member

william-ferguson-au commented Dec 24, 2015

@andryr how does having the service cause an infinite loop?

@romansl

This comment has been minimized.

Show comment
Hide comment
@romansl

romansl Dec 24, 2015

Contributor

Each process has its own Application instance.

Contributor

romansl commented Dec 24, 2015

Each process has its own Application instance.

@william-ferguson-au

This comment has been minimized.

Show comment
Hide comment
@william-ferguson-au

william-ferguson-au Dec 24, 2015

Member

@romansl are you answering my question to @andryr ?

Member

william-ferguson-au commented Dec 24, 2015

@romansl are you answering my question to @andryr ?

@romansl

This comment has been minimized.

Show comment
Hide comment
@romansl

romansl Dec 24, 2015

Contributor

My solution for this problem:

public class MyApplication extends Application {
    @Override
    public void onCreate() {
        super.onCreate();

        if (!getPackageName().equals(getAppNameByPID(android.os.Process.myPid()))) {
            return;
        }

        ACRA.init(this);
        ACRA.getErrorReporter().setReportSender(new HockeySender());

        // other initializations...
    }

    private String getAppNameByPID(int pid){
        ActivityManager manager = (ActivityManager) getSystemService(Context.ACTIVITY_SERVICE);

        for(ActivityManager.RunningAppProcessInfo processInfo : manager.getRunningAppProcesses()){
            if(processInfo.pid == pid){
                return processInfo.processName;
            }
        }

        return "";
    }
}
Contributor

romansl commented Dec 24, 2015

My solution for this problem:

public class MyApplication extends Application {
    @Override
    public void onCreate() {
        super.onCreate();

        if (!getPackageName().equals(getAppNameByPID(android.os.Process.myPid()))) {
            return;
        }

        ACRA.init(this);
        ACRA.getErrorReporter().setReportSender(new HockeySender());

        // other initializations...
    }

    private String getAppNameByPID(int pid){
        ActivityManager manager = (ActivityManager) getSystemService(Context.ACTIVITY_SERVICE);

        for(ActivityManager.RunningAppProcessInfo processInfo : manager.getRunningAppProcesses()){
            if(processInfo.pid == pid){
                return processInfo.processName;
            }
        }

        return "";
    }
}
@romansl

This comment has been minimized.

Show comment
Hide comment
@romansl

romansl Dec 24, 2015

Contributor

May be this check should be iside ACRA.init().

Contributor

romansl commented Dec 24, 2015

May be this check should be iside ACRA.init().

@william-ferguson-au

This comment has been minimized.

Show comment
Hide comment
@william-ferguson-au

william-ferguson-au Jan 3, 2016

Member

There are some problems with this implementation that need to be addressed.

Storing the ReportSender class instead of instance in ErrorReporter means that

  • ReportSender cannot be configured (there are definitely users who do this).
  • ReportSender must have a no arg constructor

Options are

  • enforce a no-arg constructor for ReportSender and disallow config, breaking N installs.
  • introduce ReportSenderFactory whose create method takes AppContext. Store the factory instead of ReportSender.
  • ?

I'm leaning towards the Factory. It's a bigger change in API but that will make people better aware of the change in requirements for a bespoke ReportSender.

Thoughts?

Member

william-ferguson-au commented Jan 3, 2016

There are some problems with this implementation that need to be addressed.

Storing the ReportSender class instead of instance in ErrorReporter means that

  • ReportSender cannot be configured (there are definitely users who do this).
  • ReportSender must have a no arg constructor

Options are

  • enforce a no-arg constructor for ReportSender and disallow config, breaking N installs.
  • introduce ReportSenderFactory whose create method takes AppContext. Store the factory instead of ReportSender.
  • ?

I'm leaning towards the Factory. It's a bigger change in API but that will make people better aware of the change in requirements for a bespoke ReportSender.

Thoughts?

@william-ferguson-au

This comment has been minimized.

Show comment
Hide comment
@william-ferguson-au

william-ferguson-au Jan 4, 2016

Member

Closed in favour of #344

Member

william-ferguson-au commented Jan 4, 2016

Closed in favour of #344

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