Skip to content
This repository has been archived by the owner on Sep 28, 2018. It is now read-only.

Retain previously added breadcrumbs whenever a new breadcrumb is added #12

Closed
wants to merge 1 commit into from

Conversation

fractalwrench
Copy link
Contributor

This changeset avoids clearing all breadcrumbs everytime a new breadcrumb is added, by omitting a call to bugsnag_event_clear_breadcrumbs. Previously this method freed memory, then allocated new memory for each breadcrumb.

Reducing this churn by retaining previously added values will partially address #10, which is caused by multiple threads attempting to free/allocate memory at the same time. I've found that running the reproduction case results in less frequent signals when this fix is applied.

One concern with this change is that we should be careful it hasn't introduced any memory leaks. I believe the bugsnag_event_add_breadcrumb method was freeing the required memory in the first place so bugsnag_event_clear_breadcrumbs wasn't necessary, but it would be good to confirm this.

@Pezzah
Copy link
Contributor

Pezzah commented Mar 19, 2018

I think we might need two different methods here, one for just adding a new breadcrumb (where we assume that the others are already in sync) and one for cloning / copying all existing breadcrumbs.

This method is not always called when adding a new breadcrumb, it is also called when initializing the NDK notifier, and also to clear all breadcrumbs (see https://github.com/bugsnag/bugsnag-android/blob/master/sdk/src/main/java/com/bugsnag/android/Client.java#L1221).

Copy link
Contributor

@kattrali kattrali left a comment

Choose a reason for hiding this comment

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

Ditto @Pezzah's comment, adding resolution of in-person conversation. The way forward is to:

  • Clear and iterate over breadcrumbs when doing the initial population and when clearBreadcrumbs() is called from the Java side
  • Separate adding a breadcrumb into a different event which only appends to the collection

@Pezzah
Copy link
Contributor

Pezzah commented Mar 21, 2018

When adding a breadcrumb will it be thread safe to assume that it is the last breadcrumb in the list that needs to be added? or does the addBreadcrumb method need to pass through parameters for the name / type?

@maltzj
Copy link

maltzj commented Apr 4, 2018

gentle poke from a concerned citizen

@maltzj
Copy link

maltzj commented Apr 18, 2018

gentle nudge

@fractalwrench
Copy link
Contributor Author

Hi @maltzj - sorry for missing this. This ticket is currently still in progress, as it has a bit wider an impact than initially thought.

@maltzj
Copy link

maltzj commented Apr 25, 2018

Thanks for the update @fractalwrench. Let me know if there's any other insight I can provide here.

@fractalwrench
Copy link
Contributor Author

Closing as this will be addressed by bugsnag/bugsnag-android#369

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants