-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
KTOR-7729 Add logging for tracking concurrent access in the dev mode #4477
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool 😎
init { | ||
if (DEVELOPMENT_MODE) { | ||
created = Throwable("ReadTask 0x${continuation.hashCode().toString(16)}").also { | ||
it.stackTraceToString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this also { it.stackTraceToString() }
doing anything here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, by default the stacktrace is not filled
|
||
init { | ||
if (DEVELOPMENT_MODE) { | ||
created = Throwable("ReadTask 0x${continuation.hashCode().toString(16)}").also { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice if we appended some diagnostic info from the coroutine context here too, since sometimes the stacktraces can be hard to follow as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't have it in the init
block, let me check what we can do
public class ConcurrentIOException( | ||
taskName: String, | ||
cause: Throwable? = null | ||
) : IllegalStateException("Concurrent $taskName attempts", cause) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we provide a @Deprecated(HIDDEN)
constructor with an old signature for binary compatibility purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, let me add
Will help with the investigation of KTOR-7729 Kotlinx-io
Check failed
fromByteChannel