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

Dictionary: include the key in the "duplicate key" exception message #1452

Merged
merged 1 commit into from Dec 24, 2015

Conversation

Projects
None yet
10 participants
@akoeplinger
Member

akoeplinger commented Aug 28, 2015

Fixes dotnet/corefx#1187
Related fix for ImmutableCollections: dotnet/corefx#185

/cc @ellismg

@OtherCrashOverride

This comment has been minimized.

Show comment
Hide comment
@OtherCrashOverride

OtherCrashOverride Aug 28, 2015

I commented in the corefx thread about why this is undesirable. As as alternative, how about adding a new field for these extended exception messages that developers may access. This would leave the current exception messages to source from digitally signed assemblies of known content while giving access to additional information for those that want it:

System.Exception.Message
System.Exception.ExtendedMessage

OtherCrashOverride commented Aug 28, 2015

I commented in the corefx thread about why this is undesirable. As as alternative, how about adding a new field for these extended exception messages that developers may access. This would leave the current exception messages to source from digitally signed assemblies of known content while giving access to additional information for those that want it:

System.Exception.Message
System.Exception.ExtendedMessage
@ayende

This comment has been minimized.

Show comment
Hide comment
@ayende

ayende Aug 28, 2015

That would be a pretty bad idea.
You can see how much problem this cause with ReflectionTypeLoadException
and the LoaderException model.
It doesn't get logged most cases, and it requires special handling
everywhere to get it working properly to actually output the full info.

Exception are exposed in very few cases:

  • Debug mode - when I want to read the exceptions full info so I can fix
    stuff.
  • Logs - in which case full information is always best.

You don't show exception information to the customer directly, so that
doesn't matter.
And having the full information there is hugely important in fixing errors.
Otherwise you get a bug report, have no idea what is going on, then have to
send an update that just add extra logging to figure it out.

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Fri, Aug 28, 2015 at 7:21 AM, OtherCrashOverride <
notifications@github.com> wrote:

I commented in the corefx thread about why this is undesirable. As as
alternative, how about adding a new field for these extended exception
messages that developers may access. This would leave the current exception
messages to source from digitally singed assemblies of known content while
giving access to additional information for those that want it:

System.Exception.Message
System.Exception.ExtendedMessage


Reply to this email directly or view it on GitHub
#1452 (comment).

ayende commented Aug 28, 2015

That would be a pretty bad idea.
You can see how much problem this cause with ReflectionTypeLoadException
and the LoaderException model.
It doesn't get logged most cases, and it requires special handling
everywhere to get it working properly to actually output the full info.

Exception are exposed in very few cases:

  • Debug mode - when I want to read the exceptions full info so I can fix
    stuff.
  • Logs - in which case full information is always best.

You don't show exception information to the customer directly, so that
doesn't matter.
And having the full information there is hugely important in fixing errors.
Otherwise you get a bug report, have no idea what is going on, then have to
send an update that just add extra logging to figure it out.

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Fri, Aug 28, 2015 at 7:21 AM, OtherCrashOverride <
notifications@github.com> wrote:

I commented in the corefx thread about why this is undesirable. As as
alternative, how about adding a new field for these extended exception
messages that developers may access. This would leave the current exception
messages to source from digitally singed assemblies of known content while
giving access to additional information for those that want it:

System.Exception.Message
System.Exception.ExtendedMessage


Reply to this email directly or view it on GitHub
#1452 (comment).

@OtherCrashOverride

This comment has been minimized.

Show comment
Hide comment
@OtherCrashOverride

OtherCrashOverride Aug 28, 2015

And having the full information there is hugely important in fixing errors.

These exception messages don't give us the full information. They are of limited use to a limited subset of real world problems. This is discussed in the corefx thread.

The actual feature wanted is a core dump that someone can attach a remote Visual Studio debug session to and see the EXACT state the program was in when it crashed. Why are we still "printf debugging"?

OtherCrashOverride commented Aug 28, 2015

And having the full information there is hugely important in fixing errors.

These exception messages don't give us the full information. They are of limited use to a limited subset of real world problems. This is discussed in the corefx thread.

The actual feature wanted is a core dump that someone can attach a remote Visual Studio debug session to and see the EXACT state the program was in when it crashed. Why are we still "printf debugging"?

@ayende

This comment has been minimized.

Show comment
Hide comment
@ayende

ayende Aug 28, 2015

@OtherCrashOverride

We can't get a crash dump from our customers. They use our software to
manage things like military installations, health care records, financial
transactions, etc.
While a crash dump is nice, it require quite a bit of expertise to use, and
as I said, in many cases, utterly impossible to get.
It also doesn't help if this particular exception happens 50,000 times a
second.

A log file, on the other hand, can either be sent, or the relevant portions
with the full error can be sent, much more easily.
It allows you to monitor distributed systems easily and quickly.

And getting the full error means that you don't need a debugging expert to
fix a bug.

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Fri, Aug 28, 2015 at 1:33 PM, OtherCrashOverride <
notifications@github.com> wrote:

And having the full information there is hugely important in fixing errors.

These exception messages don't give us the full information. They are of
limited use to a limited subset of real world problems. This is discussed
in the corefx thread.

The actual feature wanted is a core dump that someone can attach a remote
Visual Studio debug session to and see the EXACT state the program was in
when it crashed. Why are we still "printf debugging"?


Reply to this email directly or view it on GitHub
#1452 (comment).

ayende commented Aug 28, 2015

@OtherCrashOverride

We can't get a crash dump from our customers. They use our software to
manage things like military installations, health care records, financial
transactions, etc.
While a crash dump is nice, it require quite a bit of expertise to use, and
as I said, in many cases, utterly impossible to get.
It also doesn't help if this particular exception happens 50,000 times a
second.

A log file, on the other hand, can either be sent, or the relevant portions
with the full error can be sent, much more easily.
It allows you to monitor distributed systems easily and quickly.

And getting the full error means that you don't need a debugging expert to
fix a bug.

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Fri, Aug 28, 2015 at 1:33 PM, OtherCrashOverride <
notifications@github.com> wrote:

And having the full information there is hugely important in fixing errors.

These exception messages don't give us the full information. They are of
limited use to a limited subset of real world problems. This is discussed
in the corefx thread.

The actual feature wanted is a core dump that someone can attach a remote
Visual Studio debug session to and see the EXACT state the program was in
when it crashed. Why are we still "printf debugging"?


Reply to this email directly or view it on GitHub
#1452 (comment).

@OtherCrashOverride

This comment has been minimized.

Show comment
Hide comment
@OtherCrashOverride

OtherCrashOverride Aug 28, 2015

We can't get a crash dump from our customers. They use our software to manage things like military installations, health care records, financial transactions, etc.

And how do your customers feel about the runtime logging and reporting this sensitive information arbitrarily in exceptions?

OtherCrashOverride commented Aug 28, 2015

We can't get a crash dump from our customers. They use our software to manage things like military installations, health care records, financial transactions, etc.

And how do your customers feel about the runtime logging and reporting this sensitive information arbitrarily in exceptions?

@ayende

This comment has been minimized.

Show comment
Hide comment
@ayende

ayende Aug 28, 2015

What kind of sensitive information do you think are actually going on in
exceptions?

Beside, that goes into a file / system that they control. It isn't
exposed arbitrarily.
It also means that if they need to send me a file, they can go through it
very quickly.

About the most sensitive thing that I saw in exception was something like
"ProjectManhattanDesignDocs.docx is not accessible", and that is something
that they can easily filter.

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Fri, Aug 28, 2015 at 1:50 PM, OtherCrashOverride <
notifications@github.com> wrote:

We can't get a crash dump from our customers. They use our software to
manage things like military installations, health care records, financial
transactions, etc.

And how do your customers feel about the runtime logging and reporting
this sensitive information arbitrarily in exceptions?


Reply to this email directly or view it on GitHub
#1452 (comment).

ayende commented Aug 28, 2015

What kind of sensitive information do you think are actually going on in
exceptions?

Beside, that goes into a file / system that they control. It isn't
exposed arbitrarily.
It also means that if they need to send me a file, they can go through it
very quickly.

About the most sensitive thing that I saw in exception was something like
"ProjectManhattanDesignDocs.docx is not accessible", and that is something
that they can easily filter.

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Fri, Aug 28, 2015 at 1:50 PM, OtherCrashOverride <
notifications@github.com> wrote:

We can't get a crash dump from our customers. They use our software to
manage things like military installations, health care records, financial
transactions, etc.

And how do your customers feel about the runtime logging and reporting
this sensitive information arbitrarily in exceptions?


Reply to this email directly or view it on GitHub
#1452 (comment).

@OtherCrashOverride

This comment has been minimized.

Show comment
Hide comment
@OtherCrashOverride

OtherCrashOverride Aug 28, 2015

You should be able to make that decision for your company and your customers. I however should also have the same option. The information reported is useless ["Duplicate Key: System.Object" doesn't tell me anything]. It also widens the attack surface [as noted in the other thread]. I need to ensure that these types of exception messages are NEVER generated.

I hope a compromise presents.

OtherCrashOverride commented Aug 28, 2015

You should be able to make that decision for your company and your customers. I however should also have the same option. The information reported is useless ["Duplicate Key: System.Object" doesn't tell me anything]. It also widens the attack surface [as noted in the other thread]. I need to ensure that these types of exception messages are NEVER generated.

I hope a compromise presents.

@Eyas

This comment has been minimized.

Show comment
Hide comment
@Eyas

Eyas Aug 28, 2015

Why is it not viable to only generate this message in DEBUG? Or was this simply never discussed?

(i.e. change FEATURE_CORECLR to FEATURE_CORECLR && DEBUG)

Eyas commented Aug 28, 2015

Why is it not viable to only generate this message in DEBUG? Or was this simply never discussed?

(i.e. change FEATURE_CORECLR to FEATURE_CORECLR && DEBUG)

@ayende

This comment has been minimized.

Show comment
Hide comment
@ayende

ayende Aug 28, 2015

Because that doesn't help when the error happens in production.

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Fri, Aug 28, 2015 at 6:13 PM, Eyas notifications@github.com wrote:

Why is it not viable to only generate this message in DEBUG? Or was this
simply never discussed?

(i.e. change FEATURE_CORECLR to FEATURE_CORECLR && DEBUG)


Reply to this email directly or view it on GitHub
#1452 (comment).

ayende commented Aug 28, 2015

Because that doesn't help when the error happens in production.

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Fri, Aug 28, 2015 at 6:13 PM, Eyas notifications@github.com wrote:

Why is it not viable to only generate this message in DEBUG? Or was this
simply never discussed?

(i.e. change FEATURE_CORECLR to FEATURE_CORECLR && DEBUG)


Reply to this email directly or view it on GitHub
#1452 (comment).

@ayende

This comment has been minimized.

Show comment
Hide comment
@ayende

ayende Aug 28, 2015

How often do you think people just put plain Object as a dictionary key?
In most cases, it is something that has a sensible ToString() impl.

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Fri, Aug 28, 2015 at 2:03 PM, OtherCrashOverride <
notifications@github.com> wrote:

You should be able to make that decision for your company and your
customers. I however should also have the same option. The information
reported is useless ["Duplicate Key: System.Object" doesn't tell me
anything]. It also widens the attack surface [as noted in the other
thread]. I need to ensure that these types of exception messages are NEVER
generated.

I hope a compromise presents.


Reply to this email directly or view it on GitHub
#1452 (comment).

ayende commented Aug 28, 2015

How often do you think people just put plain Object as a dictionary key?
In most cases, it is something that has a sensible ToString() impl.

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Fri, Aug 28, 2015 at 2:03 PM, OtherCrashOverride <
notifications@github.com> wrote:

You should be able to make that decision for your company and your
customers. I however should also have the same option. The information
reported is useless ["Duplicate Key: System.Object" doesn't tell me
anything]. It also widens the attack surface [as noted in the other
thread]. I need to ensure that these types of exception messages are NEVER
generated.

I hope a compromise presents.


Reply to this email directly or view it on GitHub
#1452 (comment).

@Eyas

This comment has been minimized.

Show comment
Hide comment
@Eyas

Eyas Aug 28, 2015

But in production we often lose a lot of debugging ability anyway; debug symbols are missing, the generated code is optimized and more difficult to traverse, crash dumps are less useful, or, in your case, customers are not willing to share them anyway. A better message seems like it could save some debugging time, so at a minimum, it is definitely useful with DEBUG on.

You're also saying its useful without DEBUG on. Sure. But some people are raising 'security' concerns.

I don't know what the outcome of the debate will be. But if the conclusion is that security concerns outweigh the gain in productivity, then I hope the resulting decision is to at least still implement this in, say, DEBUG.

Eyas commented Aug 28, 2015

But in production we often lose a lot of debugging ability anyway; debug symbols are missing, the generated code is optimized and more difficult to traverse, crash dumps are less useful, or, in your case, customers are not willing to share them anyway. A better message seems like it could save some debugging time, so at a minimum, it is definitely useful with DEBUG on.

You're also saying its useful without DEBUG on. Sure. But some people are raising 'security' concerns.

I don't know what the outcome of the debate will be. But if the conclusion is that security concerns outweigh the gain in productivity, then I hope the resulting decision is to at least still implement this in, say, DEBUG.

@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Aug 28, 2015

Member

then I hope the resulting decision is to at least still implement this in, say, DEBUG.

Just to be clear, using a DEBUG compilation constant in mscorlib would only help if you were debugging your application using a debug build of CoreCLR and mscorlib; it wouldn't matter whether your application was a debug or release build.

Member

stephentoub commented Aug 28, 2015

then I hope the resulting decision is to at least still implement this in, say, DEBUG.

Just to be clear, using a DEBUG compilation constant in mscorlib would only help if you were debugging your application using a debug build of CoreCLR and mscorlib; it wouldn't matter whether your application was a debug or release build.

@ayende

This comment has been minimized.

Show comment
Hide comment
@ayende

ayende Aug 28, 2015

You can deploy with PDB, that is a pretty standard behavior, since stack
traces are much better then.
Sure, you might get methods optimized away, but it is still clear.

If they have security concerns, don't show exceptions to users. You
shouldn't anyway if this is something that concerns you.

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Fri, Aug 28, 2015 at 6:49 PM, Eyas notifications@github.com wrote:

But in production we often lose a lot of debugging ability anyway; debug
symbols are missing, the generated code is optimized and more difficult to
traverse, crash dumps are less useful, or, in your case, customers are not
willing to share them anyway. A better message seems like it could save
some debugging time, so at a minimum, it is definitely useful with DEBUG
on.

You're also saying its useful without DEBUG on. Sure. But some people are
raising 'security' concerns.

I don't know what the outcome of the debate will be. But if the conclusion
is that security concerns outweigh the gain in productivity, then I hope
the resulting decision is to at least still implement this in, say, DEBUG.


Reply to this email directly or view it on GitHub
#1452 (comment).

ayende commented Aug 28, 2015

You can deploy with PDB, that is a pretty standard behavior, since stack
traces are much better then.
Sure, you might get methods optimized away, but it is still clear.

If they have security concerns, don't show exceptions to users. You
shouldn't anyway if this is something that concerns you.

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Fri, Aug 28, 2015 at 6:49 PM, Eyas notifications@github.com wrote:

But in production we often lose a lot of debugging ability anyway; debug
symbols are missing, the generated code is optimized and more difficult to
traverse, crash dumps are less useful, or, in your case, customers are not
willing to share them anyway. A better message seems like it could save
some debugging time, so at a minimum, it is definitely useful with DEBUG
on.

You're also saying its useful without DEBUG on. Sure. But some people are
raising 'security' concerns.

I don't know what the outcome of the debate will be. But if the conclusion
is that security concerns outweigh the gain in productivity, then I hope
the resulting decision is to at least still implement this in, say, DEBUG.


Reply to this email directly or view it on GitHub
#1452 (comment).

@Eyas

This comment has been minimized.

Show comment
Hide comment
@Eyas

Eyas Aug 28, 2015

@stephentoub,

Just to be clear, using a DEBUG compilation constant in mscorlib would only help if you were debugging your application using a debug build of CoreCLR and mscorlib

bleh, you're right. Compile-time checks wouldn't work. I wonder if there's a runtime check that would achieve the effect I'm talking about. I assume we don't care too much about performance when throwing exceptions (a branch/variable check seems insignificant compared to throwing), but maybe we do.

Eyas commented Aug 28, 2015

@stephentoub,

Just to be clear, using a DEBUG compilation constant in mscorlib would only help if you were debugging your application using a debug build of CoreCLR and mscorlib

bleh, you're right. Compile-time checks wouldn't work. I wonder if there's a runtime check that would achieve the effect I'm talking about. I assume we don't care too much about performance when throwing exceptions (a branch/variable check seems insignificant compared to throwing), but maybe we do.

@akoeplinger

This comment has been minimized.

Show comment
Hide comment
@akoeplinger

akoeplinger Aug 28, 2015

Member

@Eyas making this debug-only would severely reduce the usability of this change. As @ayende notes, it's often impossible/hard to reproduce certain failures during debugging and log files are your only hope of understanding what went wrong. Every little bit of additional information helps.

Member

akoeplinger commented Aug 28, 2015

@Eyas making this debug-only would severely reduce the usability of this change. As @ayende notes, it's often impossible/hard to reproduce certain failures during debugging and log files are your only hope of understanding what went wrong. Every little bit of additional information helps.

@masonwheeler

This comment has been minimized.

Show comment
Hide comment
@masonwheeler

masonwheeler Aug 28, 2015

I've seen this line of "reasoning" before. @OtherCrashOverride does not appear to understand the difference between server software, in which the software is run by the developer and the end-user can be an attacker, and client-side software, in which the software is already in the possession of the end-user, who is therefore, by definition, not an attacker.

His objections almost make sense in the server-side world... except that a trivial solution exists: just don't show error messages to the end-user. Because server-side software is run by developers, not end-users, we can dump all necessary error messages to locations where we can see the bugs and we don't have to show them to the users.

But client-side software is a completely different beast. The software is under the control of someone other than the developers, which means that when something goes wrong, the person who has possession of the software (usually) has no way to fix it, and the people who have the means to fix it do not have possession of the instance of the software that is erroring out. This makes traditional debugging techniques impossible, which makes it imperative that bug reports contain as much relevant information as possible, in order for the developers to reproduce the problem and be able to fix it.

This, specifically, is the use case, the problem that this PR solves, and it's definitely worth solving. In the server-side cases that OtherCrashOverride is worried about, again, the solution is trivial: just don't show the user your exception messages in the first place. But even if you're not showing it to the user, you still want that information for yourself. So yes, let's merge this.

masonwheeler commented Aug 28, 2015

I've seen this line of "reasoning" before. @OtherCrashOverride does not appear to understand the difference between server software, in which the software is run by the developer and the end-user can be an attacker, and client-side software, in which the software is already in the possession of the end-user, who is therefore, by definition, not an attacker.

His objections almost make sense in the server-side world... except that a trivial solution exists: just don't show error messages to the end-user. Because server-side software is run by developers, not end-users, we can dump all necessary error messages to locations where we can see the bugs and we don't have to show them to the users.

But client-side software is a completely different beast. The software is under the control of someone other than the developers, which means that when something goes wrong, the person who has possession of the software (usually) has no way to fix it, and the people who have the means to fix it do not have possession of the instance of the software that is erroring out. This makes traditional debugging techniques impossible, which makes it imperative that bug reports contain as much relevant information as possible, in order for the developers to reproduce the problem and be able to fix it.

This, specifically, is the use case, the problem that this PR solves, and it's definitely worth solving. In the server-side cases that OtherCrashOverride is worried about, again, the solution is trivial: just don't show the user your exception messages in the first place. But even if you're not showing it to the user, you still want that information for yourself. So yes, let's merge this.

@ayende

This comment has been minimized.

Show comment
Hide comment
@ayende

ayende Aug 28, 2015

@masonwheeler
Note that in both cases, for client side & server side, having this
additional information is important.

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Fri, Aug 28, 2015 at 10:42 PM, masonwheeler notifications@github.com
wrote:

I've seen this line of "reasoning" before. @OtherCrashOverride
https://github.com/OtherCrashOverride does not appear to understand the
difference between server software, in which the software is run by the
developer and the end-user can be an attacker, and client-side software, in
which the software is already in the possession of the end-user, who is
therefore, by definition, not an attacker.

His objections almost make sense in the server-side world... except
that a trivial solution exists: just don't show error messages to the
end-user
. Because server-side software is run by developers, not
end-users, we can dump all necessary error messages to locations where we
can see the bugs and we don't have to show them to the users.

But client-side software is a completely different beast. The software is
under the control of someone other than the developers, which means that
when something goes wrong, the person who has possession of the software
(usually) has no way to fix it, and the people who have the means to fix it
do not have possession of the instance of the software that is erroring
out. This makes traditional debugging techniques impossible, which makes it
imperative that bug reports contain as much relevant information as
possible, in order for the developers to reproduce the problem and be able
to fix it.

This, specifically, is the use case, the problem that this PR solves, and
it's definitely worth solving. In the server-side cases that
OtherCrashOverride is worried about, again, the solution is trivial: just
don't show the user your exception messages in the first place. But even if
you're not showing it to the user, you still want that information for
yourself. So yes, let's merge this.


Reply to this email directly or view it on GitHub
#1452 (comment).

ayende commented Aug 28, 2015

@masonwheeler
Note that in both cases, for client side & server side, having this
additional information is important.

*Hibernating Rhinos Ltd *

Oren Eini* l CEO l *Mobile: + 972-52-548-6969

Office: +972-4-622-7811 *l *Fax: +972-153-4-622-7811

On Fri, Aug 28, 2015 at 10:42 PM, masonwheeler notifications@github.com
wrote:

I've seen this line of "reasoning" before. @OtherCrashOverride
https://github.com/OtherCrashOverride does not appear to understand the
difference between server software, in which the software is run by the
developer and the end-user can be an attacker, and client-side software, in
which the software is already in the possession of the end-user, who is
therefore, by definition, not an attacker.

His objections almost make sense in the server-side world... except
that a trivial solution exists: just don't show error messages to the
end-user
. Because server-side software is run by developers, not
end-users, we can dump all necessary error messages to locations where we
can see the bugs and we don't have to show them to the users.

But client-side software is a completely different beast. The software is
under the control of someone other than the developers, which means that
when something goes wrong, the person who has possession of the software
(usually) has no way to fix it, and the people who have the means to fix it
do not have possession of the instance of the software that is erroring
out. This makes traditional debugging techniques impossible, which makes it
imperative that bug reports contain as much relevant information as
possible, in order for the developers to reproduce the problem and be able
to fix it.

This, specifically, is the use case, the problem that this PR solves, and
it's definitely worth solving. In the server-side cases that
OtherCrashOverride is worried about, again, the solution is trivial: just
don't show the user your exception messages in the first place. But even if
you're not showing it to the user, you still want that information for
yourself. So yes, let's merge this.


Reply to this email directly or view it on GitHub
#1452 (comment).

@OtherCrashOverride

This comment has been minimized.

Show comment
Hide comment
@OtherCrashOverride

OtherCrashOverride Aug 28, 2015

@OtherCrashOverride does not appear to understand the difference between server software, in which the software is run by the developer and the end-user can be an attacker, and client-side software, in which the software is already in the possession of the end-user, who is therefore, by definition, not an attacker.

I know him personally and I can tell you for a fact that @OtherCrashOverride does understand a great deal about a great many things; server and client-side software included!

OtherCrashOverride commented Aug 28, 2015

@OtherCrashOverride does not appear to understand the difference between server software, in which the software is run by the developer and the end-user can be an attacker, and client-side software, in which the software is already in the possession of the end-user, who is therefore, by definition, not an attacker.

I know him personally and I can tell you for a fact that @OtherCrashOverride does understand a great deal about a great many things; server and client-side software included!

@sharwell

This comment has been minimized.

Show comment
Hide comment
@sharwell

sharwell Sep 1, 2015

Member

I remain against this proposal for the following reasons:

  1. key.ToString() is occasionally a meaningful representation of the data used for determining key equality, but it is also frequently meaningless. Including it for the latter cases would be misleading.
  2. key.ToString() may be a expensive operation (calculation cost or produce a very large string in memory). While each of these may be transient at runtime, the latter could cause unexpected flooding of existing logging databases.
  3. I continue to question the security ramifications of this change, and do not believe the benefits outweigh the risk.

I would likely be in support of an alternate proposal that makes the duplicate key available through some other means, provided the value was not included in the result of Message or ToString() by default.

Member

sharwell commented Sep 1, 2015

I remain against this proposal for the following reasons:

  1. key.ToString() is occasionally a meaningful representation of the data used for determining key equality, but it is also frequently meaningless. Including it for the latter cases would be misleading.
  2. key.ToString() may be a expensive operation (calculation cost or produce a very large string in memory). While each of these may be transient at runtime, the latter could cause unexpected flooding of existing logging databases.
  3. I continue to question the security ramifications of this change, and do not believe the benefits outweigh the risk.

I would likely be in support of an alternate proposal that makes the duplicate key available through some other means, provided the value was not included in the result of Message or ToString() by default.

@OtherCrashOverride

This comment has been minimized.

Show comment
Hide comment
@OtherCrashOverride

OtherCrashOverride Sep 1, 2015

I would likely be in support of an alternate proposal that makes the duplicate key available through some other means, provided the value was not included in the result of Message or ToString() by default.

This is being discussed in dotnet/corefx#3025

OtherCrashOverride commented Sep 1, 2015

I would likely be in support of an alternate proposal that makes the duplicate key available through some other means, provided the value was not included in the result of Message or ToString() by default.

This is being discussed in dotnet/corefx#3025

@@ -53,6 +53,12 @@ internal static class ThrowHelper {
throw new ArgumentException(Environment.GetResourceString("Arg_WrongType", value, targetType), "value");
}
#if FEATURE_CORECLR
internal static void ThrowAddingDuplicateWithKeyArgumentException(object key) {
throw new ArgumentException(Environment.GetResourceString("Argument_AddingDuplicateWithKey", key));

This comment has been minimized.

@stephentoub

stephentoub Sep 16, 2015

Member

What happens if key.ToString() throws?

@stephentoub

stephentoub Sep 16, 2015

Member

What happens if key.ToString() throws?

This comment has been minimized.

@akoeplinger

akoeplinger Sep 16, 2015

Member

That's evil 😆

See also @nguerrera's comment in dotnet/corefx#185 (comment)

@akoeplinger

akoeplinger Sep 16, 2015

Member

That's evil 😆

See also @nguerrera's comment in dotnet/corefx#185 (comment)

@weshaggard

This comment has been minimized.

Show comment
Hide comment
@weshaggard

weshaggard Dec 24, 2015

Member

@akoeplinger thanks for fixing this and sorry it has been sitting here for so long. The change LGTM.

Member

weshaggard commented Dec 24, 2015

@akoeplinger thanks for fixing this and sorry it has been sitting here for so long. The change LGTM.

weshaggard added a commit that referenced this pull request Dec 24, 2015

Merge pull request #1452 from akoeplinger/dictionary-duplicatekey
Dictionary: include the key in the "duplicate key" exception message

@weshaggard weshaggard merged commit e79ae8c into dotnet:master Dec 24, 2015

1 check passed

default Build finished. No test results found.
Details

@akoeplinger akoeplinger deleted the akoeplinger:dictionary-duplicatekey branch Dec 24, 2015

@akoeplinger

This comment has been minimized.

Show comment
Hide comment
@akoeplinger

akoeplinger Dec 24, 2015

Member

😄 This is a pretty awesome christmas present, thanks @weshaggard ! 🎄 🎉 🎁

Member

akoeplinger commented Dec 24, 2015

😄 This is a pretty awesome christmas present, thanks @weshaggard ! 🎄 🎉 🎁

jkotas added a commit to jkotas/corefx that referenced this pull request Dec 26, 2015

Include the key in the "duplicate key" exception message
Port dotnet/coreclr#1452 to CoreRT/.NET Native clone of Dictionary, as well as other collections implemented in System.Collections.dll.

jkotas added a commit to jkotas/corefx that referenced this pull request Dec 26, 2015

Include the key in the "duplicate key" exception message
Port dotnet/coreclr#1452 to CoreRT/.NET Native clone of Dictionary, as well as other collections implemented in System.Collections.dll.

jkotas added a commit to jkotas/corefx that referenced this pull request Dec 27, 2015

Include the key in the "duplicate key" exception message
Port dotnet/coreclr#1452 to CoreRT/.NET Native clone of Dictionary, as well as other collections implemented in System.Collections.dll.

jkotas added a commit to jkotas/corert that referenced this pull request Dec 30, 2015

Include the key in the "duplicate key" exception message
Port dotnet/coreclr#1452 to CoreRT. Also deleted redundant resource files.

@ghost ghost referenced this pull request Jan 5, 2016

Closed

Add key value to KeyNotFoundException #5188

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment