-
Notifications
You must be signed in to change notification settings - Fork 55
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
Conversation
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. |
@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. |
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). |
I'm closing this stale PR due to BFG repo cleanup. Please reopen it when ready. |
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.