Skip to content

Remove outdated UploadNow comments #190

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

Closed
wants to merge 4 commits into from

Conversation

bliptec
Copy link
Contributor

@bliptec bliptec commented Nov 19, 2019

We couldn't find the conversation, but @HIROSN and I believe there was a conversation where it was mentioned that it's fine to directly call UploadNow within the debug callback now. Updated the code to reflect that.

@maxgolov
Copy link
Contributor

maxgolov commented Nov 21, 2019

I reran the checks: LogManager_KilledEventsAreDropped appears to be very intermittently failing due to server "black-holing" bad events from time to time (returning 200 OK instead of 401 with Kill-Tokens header). I will rework that test.. #150 ... Your code is fine.

@maxgolov
Copy link
Contributor

@bliptec - do you want to merge this? My only worry is we should not create an assumption that other SDK API methods are safe to be called from within the Event Callback. This will result in some problems if some other API method gets added into the same callback side-by-side with UploadNow. SDK invokes the callback synchronously on its own internal thread. If customer does something bad internally within that same thread, e.g. recursively calling some SDK API that uses a mutex rather than recursive_mutex, bad things would start happening.

UploadNow - itself being an async by design, and shielded by recursive_mutex is rather an exception, being safe-by-coincidence rather than being safe-by-design. Original design was not to allow any invocations of the SDK API methods from event callback.

@maxgolov
Copy link
Contributor

maxgolov commented Mar 10, 2020

I'd like to avoid merging this for now --- we need to add some clarifications about what can and what cannot be done in SDK callback, and what SDK is expecting from something that is run inside the callback. We discussed some hypothetical scenarios in code review of #266 , where Network Detector network state change notification may potentially come at a moment that may get stuck in customer code on LogManager lock. Let's discuss how to cover this.... the easiest would be, of course, to do all actions that deal with SDK API surface outside of callback, i.e. in a separate thread... OR orchestrate invocations of certain methods (UploadNow vs. FlushAndTeardown), having the knowledge of what the app is doing, i.e. if FlushAndTeardown is invoked, avoid calling into UploadNow from user code inside SDK callback, as that may potentially lead to a deadlock (although we need to verify if that's the case).

@bliptec bliptec added the do not merge PRs that are not ready to merge label Mar 18, 2020
@maxgolov
Copy link
Contributor

I'm closing this stale PR due to BFG repo cleanup. Please reopen it when ready.

@maxgolov maxgolov closed this Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge PRs that are not ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants