Skip to content
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

Question about the root.detachAndStopAllAppenders() call #64

Closed
Ldoppea opened this issue Feb 28, 2024 · 8 comments · Fixed by #66 or #69
Closed

Question about the root.detachAndStopAllAppenders() call #64

Ldoppea opened this issue Feb 28, 2024 · 8 comments · Fixed by #66 or #69

Comments

@Ldoppea
Copy link

Ldoppea commented Feb 28, 2024

Hi,

First, thank you for this project, I started using it a few days ago and it makes way easier to get debug data from the app's testers

I'm investigating an incompatibility between react-native-file-logger and react-native-background-geolocation which has its own log mechanism, also based on logback-android

When using both libraries, then the geolocation library stops working

I just found that this is because react-native-file-logger calls root.detachAndStopAllAppenders() during its initialization. Due to this, the react-native-background-geolocation's appenders are cleared and so it cannot do its job anymore

I tried to remove this line locally and it fixes the problem

The question is now: why is this line needed?

The project's history says that it has been added in this specific commit: 5c04853

Because it was not initially there and because it has been added later, my conclusion is that it is here for a specific purpose (i.e. to fix a bug) and so I don't want to remove it without understanding why it was here in the first place

So my two question are:

  • Do you remember why it was needed?
  • Based on first answer, do you think I should remove this in a local patch? or to make a PR on this repo?

Thanks

@fdrault
Copy link
Member

fdrault commented Feb 29, 2024

Hello,

Thank you for the detailed issue

Do you remember why it was needed?

I don't have the history, but I think the purpose is re-reading configuration information while keeping only one appender at once. It is needed for dynamic configuration change.

Based on first answer, do you think I should remove this in a local patch? or to make a PR on this repo?

If you call configure() only once, it should works in your configuration, so you can use it in a local patch with that in mind, but you are breaking things in this library, so "remove" the line is not a proper fix

If you want to work on a real solution, any PR is welcome, we are open to contributions 👍

Ldoppea added a commit to cozy/cozy-flagship-app that referenced this issue Feb 29, 2024
In #1137 we implemented a new File logger mechanism

Unfortunately this has a side effect on
react-native-background-geolocation which has a logger mechanism based
on the same library and fails to work if we enable our new File logger

A solution is investigated in BeTomorrow/react-native-file-logger#64
but until then we want to disable the feature so we can continue
publishing the app

Related PR: #1137
Related Issue: BeTomorrow/react-native-file-logger#64
Ldoppea added a commit to cozy/cozy-flagship-app that referenced this issue Feb 29, 2024
In #1137 we implemented a new File logger mechanism

Unfortunately this has a side effect on
react-native-background-geolocation which has a logger mechanism based
on the same library and fails to work if we enable our new File logger

A solution is investigated in BeTomorrow/react-native-file-logger#64
but until then we want to disable the feature so we can continue
publishing the app

Related PR: #1137
Related Issue: BeTomorrow/react-native-file-logger#64
@Ldoppea
Copy link
Author

Ldoppea commented Feb 29, 2024

If you want to work on a real solution, any PR is welcome, we are open to contributions 👍

For sure, I would be happy to contribute

It is needed for dynamic configuration change.

Do you mean if the developper want to call configure() multiple times based on some events?

For now I see two potential solutions.

First one would be to add a parameter resetConfig (or something similar) to the configure() method that would allow the dev to tell if they want to do the detachAndStopAllAppenders thing or not. So we can keep the current behavior but allowing to change it for other libs compatibility

Second one would be to find a way to detach only the react-native-file-logger appenders (if they exist). I prefer this one but I don't know yet if this is possible. There is a detachAppender(String name) method that may help. But there is no mention of the stop action that seems to exist in the detachAndStopAllAppenders() one, so I'm not sure yet if this would help.

@fdrault
Copy link
Member

fdrault commented Mar 7, 2024

Second one would be to find a way to detach only the react-native-file-logger appenders (if they exist). I prefer this one but I don't know yet if this is possible. There is a detachAppender(String name) method that may help. But there is no mention of the stop action that seems to exist in the detachAndStopAllAppenders() one, so I'm not sure yet if this would help.

I tried this idea in the Pull Request #66 .

I have publish the fixed version 0.5.4-rc.1 with tags "next".

@Ldoppea Can you install this version and tell me if it works better with react-native-background-geolocation ?

npm install react-native-file-logger@next

@Ldoppea
Copy link
Author

Ldoppea commented Mar 8, 2024

Hi,

Thanks for the time you spent on this 👍

My project is using an old ReactNative version so I'm stuck with v0.4.1 and I cannot test 0.5.4-rc.1

But I did the test by applying the same modification and it works very well 😃

Ldoppea added a commit to cozy/cozy-flagship-app that referenced this issue Mar 8, 2024
In #1183 we disabled the new File logger mechanism due to a side effect
with react-native-background-geolocation

This was because react-native-file-logger would reset all log appenders
during initialisation which would prevent native-background-geolocation
to work

This commit applies the fix from BeTomorrow/react-native-file-logger#66
(we cannot update the lib as we are still using ReactNative old
architecture)

Related PR: #1183
Related issue: BeTomorrow/react-native-file-logger#64
Related issue: BeTomorrow/react-native-file-logger#66
@fdrault
Copy link
Member

fdrault commented Mar 11, 2024

Hi,

Thanks for the time you spent on this 👍

My project is using an old ReactNative version so I'm stuck with v0.4.1 and I cannot test 0.5.4-rc.1

But I did the test by applying the same modification and it works very well 😃

I see, thanks for reporting this.
Being up-to-date is kinda hard on react-native, but I don't have time to ensure backward compatibilities, still I am glad the patch worked for you 😄

Ldoppea added a commit to cozy/cozy-flagship-app that referenced this issue Mar 21, 2024
In #1183 we disabled the new File logger mechanism due to a side effect
with react-native-background-geolocation

This was because react-native-file-logger would reset all log appenders
during initialisation which would prevent native-background-geolocation
to work

This commit applies the fix from BeTomorrow/react-native-file-logger#66
(we cannot update the lib as we are still using ReactNative old
architecture)

Related PR: #1183
Related issue: BeTomorrow/react-native-file-logger#64
Related issue: BeTomorrow/react-native-file-logger#66
Ldoppea added a commit to cozy/cozy-flagship-app that referenced this issue Mar 21, 2024
In #1183 we disabled the new File logger mechanism due to a side effect
with react-native-background-geolocation

This was because react-native-file-logger would reset all log appenders
during initialisation which would prevent native-background-geolocation
to work

This commit applies the fix from BeTomorrow/react-native-file-logger#66
(we cannot update the lib as we are still using ReactNative old
architecture)

Related PR: #1183
Related issue: BeTomorrow/react-native-file-logger#64
Related issue: BeTomorrow/react-native-file-logger#66
@Ldoppea
Copy link
Author

Ldoppea commented Apr 3, 2024

hi @fdrault sorry i would like to reopen this issue,

I just found that the iOS part has a similar bug as it also call removeAllLoggers here

The difference with the android part is that there is no visible exception. In our app, the first logger has enough time to be created, then to logs some lines, and finally it is removed by removeAllLoggers. We didn't realize it because we just checked that the log file was existing with some logs, and so we missed the fact that logs stopped after a few seconds (or milliseconds)

I wanted to make a PR with a fix, but unfortunately, CocoaLumberjack doesn't seems to allow removing a logger by name like logback-android does. The removeLogger methods seems to only take a reference to an existing logger but I did not find any method that take a string as an ID.

The allLogger method may be useful too, but here again I don't see any way to distinguish between the RNFileLogger's loggers and others.

Do you have any idea on how to handle this?

@fdrault
Copy link
Member

fdrault commented Apr 8, 2024

I am re-opening the issue, thanks for your research
I will look into it when I have some time

@fdrault fdrault reopened this Apr 8, 2024
Ldoppea added a commit to cozy/cozy-flagship-app that referenced this issue Apr 13, 2024
In #1187 we fixed the new File logger initialization on Android as it
would break the react-native-background-geolocation plugin's internal
File logger

This was because react-native-file-logger would reset all log appenders
during initialisation which would prevent native-background-geolocation
to work

This bug was also present in iOS but appeared latter so we did not
notice it and so we did not fix it on initial PR

This commit applies the fix from BeTomorrow/react-native-file-logger#69
(we cannot update the lib as we are still using ReactNative old
architecture)

Related PR: #1187
Related PR: #1183
Related issue: BeTomorrow/react-native-file-logger#64
Related issue: BeTomorrow/react-native-file-logger#66
Related issue: BeTomorrow/react-native-file-logger#69
@Ldoppea
Copy link
Author

Ldoppea commented Apr 13, 2024

Hi,

I just tested @alois-beto 's fix on my project and it works like a charm. I didn't expect the fix to be that simple, congrats 😅

Thank you for your help @alois-beto and @fdrault 👍

Ldoppea added a commit to cozy/cozy-flagship-app that referenced this issue Apr 15, 2024
In #1187 we fixed the new File logger initialization on Android as it
would break the react-native-background-geolocation plugin's internal
File logger

This was because react-native-file-logger would reset all log appenders
during initialisation which would prevent native-background-geolocation
to work

This bug was also present in iOS but appeared latter so we did not
notice it and so we did not fix it on initial PR

This commit applies the fix from BeTomorrow/react-native-file-logger#69
(we cannot update the lib as we are still using ReactNative old
architecture)

Related PR: #1187
Related PR: #1183
Related issue: BeTomorrow/react-native-file-logger#64
Related issue: BeTomorrow/react-native-file-logger#66
Related issue: BeTomorrow/react-native-file-logger#69
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants