[PlaygroundLogger] Add checks in the logger entrypoints to prevent recursive logging. #29
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Added a thread-local `bool`, `PGLThreadIsLogging`, which is used in each of the logger entrypoints. This is defined in C as it's not possible (to my knowledge) to create a thread-local variable in Swift. Each of the logger entrypoints first checks this value; if it's `true`, then the function returns nil. If it's `false`, then the function sets it to `true`, defers setting it back to `false`, and then generates the log packet. This allows logging to complete successfully in cases when logging a value would cause that value to be logged (e.g. if the implementation of `CustomStringConvertible` or `CustomPlaygroundDisplayConvertible` caused `self` to be logged). This addresses <rdar://problem/41460357> / SR-8349.
Added tests for both the new and legacy entrypoints to ensure that recursive logging does not occur. In both cases, a struct whose implementation of `CustomStringConvertible` logs `self` is logged. Without the changes in the previous commit, this causes infinite recursion. With those changes, these tests pass as expected. This is for <rdar://problem/41460357> / SR-8349.
jonathanpenn
approved these changes
Sep 1, 2018
jonathanpenn
left a comment
jonathanpenn
left a comment
There was a problem hiding this comment.
Looks great! I had one very minor comment on how a comment was worded. I think this is good to go.
| /// logging. | ||
| /// | ||
| /// This is used by the functions in LoggerEntrypoints.swift and | ||
| /// LegacyEntrypoints.swift to prevent generating log packets while in already |
There was a problem hiding this comment.
Very minor nit, but the way I'm reading this it seems like "...while in already..." should be "...while already...".
|
@rjmccall Does Swift handle |
Member
|
No, I don't think we have any logic for handling thread-locals. |
|
Okay. To be safe, Connor, you should write some static inline C wrappers around your |
Member
Author
|
Ah, OK. In email @jckarter suggested that using |
…d set `PGLThreadIsLogging`. This is to insulate from any issues which might arise because Swift may or may not handle `__thread` variables correctly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The logger entrypoints now consult a C-defined thread-local
booland exit immediately if the current thread is already logging. If the current thread is not logging, they set theboolto indicate that it is, and then unset it at the end of the function.This allows types which reference
selfin their implementations ofCustomPlaygroundQuickLookableorCustomStringConvertible(among other things) in such a way thatselfwould be logged to not cause infinite recursion in the logger. It also means that logging won't log the side-effects of logging anymore.This addresses rdar://problem/41460357 / SR-8349.