You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Rupert Smith <ru...@googlemail.com> on 2007/05/18 14:09:24 UTC
More bad exception code...
Just found this shocker:
private static final ExistingSubscriptionPreventsExclusive
EXISTING_SUBSCRIPTION =
new ExistingSubscriptionPreventsExclusive();
Looks like a really bad idea as doesn't seem thread safe, as mutliple
threads raising these exceptions will overwrite each others stacks. I not
even sure that throw is thread safe in that respect? This could also cause
memory leaks, because objects refed by the static stack dump won't be
garbage collected.
Rupert
Re: More bad exception code...
Posted by Rupert Smith <ru...@googlemail.com>.
Ok, Rob has convinced me he is right. The stack trace gets filled in on the
constructor, 'fillInStackTrace' is available for bringing it up to date if
needed. Makes sense, or else catch and re-throw would lose the stack trace
below the catch point.
On 18/05/07, Rupert Smith <ru...@googlemail.com> wrote:
>
> Is that really the case? Then the stack trace wouldn't take you back to
> the throw point, but to the creation point? I've found opinions that state
> both cases. Going to have to look that one up I think...
>
> On 18/05/07, Robert Godfrey <ro...@gmail.com> wrote:
> >
> > The stack trace is generated on Exception creation, not on throwing...
> > so it is completely safe. The stack trace in that exception would be
> > the stack at class instantiation.
> >
> > This sort of trick is normally used where the exception is thrown
> > quite frequently in a semi- performance critical piece of code. I'm
> > not sure that was really an issue here... but be careful with this
> > sort of re-factoring (obviously there should hae been a comment about
> > why this was there)...
> >
> > - Rob
> >
> > On 18/05/07, Rupert Smith <ru...@googlemail.com> wrote:
> > > Well, I'm actually not sure that the stack trace really does hold
> > references
> > > to the objects in it, or just a listing of the class names and line
> > numbers.
> > > Also do exceptions hold the stack traces on a per-thread basis, as if
> > they
> > > are thread locals? Again I'm not sure, but if they do then multiple
> > threads
> > > throwing the same exception instance simultaneously will work happily.
> > >
> > > Anyway, I've refactored out the *ugly* code, so I don't have to puzzle
> > about
> > > these things.
> > >
> > > Rupert
> > >
> > > On 18/05/07, Robert Godfrey <ro...@gmail.com> wrote:
> > > >
> > > > Absolutely !
> > > >
> > > > On 18/05/07, John O'Hara < john.r.ohara@gmail.com> wrote:
> > > > > Perhaps worth some comments to that effect, given it is a bit
> > wierd.
> > > > >
> > > > > On 18/05/07, Robert Godfrey < rob.j.godfrey@gmail.com> wrote:
> > > > > >
> > > > > > No - its a single instance of the Exception. So there's no
> > leak...
> > > > > > and the stack is not interesting... and there's no thread
> > safety
> > > > > > issue
> > > > > >
> > > > > > The exception is used to flag a particular type of exception
> > which
> > > > > > leads to an AMQP error
> > > > > > There's no need to have the stack trace retained.
> > > > > >
> > > > > > It's just like having enumerated error codes returned.
> > > > > >
> > > > > > It's *ugly* code, I'll grant you that :-)
> > > > > >
> > > > > >
> > > > > > -- Rob
> > > > > >
> > > > > > On 18/05/07, Rupert Smith <ru...@googlemail.com> wrote:
> > > > > > > Just found this shocker:
> > > > > > >
> > > > > > > private static final ExistingSubscriptionPreventsExclusive
> > > > > > > EXISTING_SUBSCRIPTION =
> > > > > > > new ExistingSubscriptionPreventsExclusive();
> > > > > > >
> > > > > > > Looks like a really bad idea as doesn't seem thread safe, as
> > > > mutliple
> > > > > > > threads raising these exceptions will overwrite each others
> > stacks.
> > > > I
> > > > > > not
> > > > > > > even sure that throw is thread safe in that respect? This
> > could also
> > > > > > cause
> > > > > > > memory leaks, because objects refed by the static stack dump
> > won't
> > > > be
> > > > > > > garbage collected.
> > > > > > >
> > > > > > > Rupert
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
>
Re: More bad exception code...
Posted by Rupert Smith <ru...@googlemail.com>.
Is that really the case? Then the stack trace wouldn't take you back to the
throw point, but to the creation point? I've found opinions that state both
cases. Going to have to look that one up I think...
On 18/05/07, Robert Godfrey <ro...@gmail.com> wrote:
>
> The stack trace is generated on Exception creation, not on throwing...
> so it is completely safe. The stack trace in that exception would be
> the stack at class instantiation.
>
> This sort of trick is normally used where the exception is thrown
> quite frequently in a semi- performance critical piece of code. I'm
> not sure that was really an issue here... but be careful with this
> sort of re-factoring (obviously there should hae been a comment about
> why this was there)...
>
> - Rob
>
> On 18/05/07, Rupert Smith <ru...@googlemail.com> wrote:
> > Well, I'm actually not sure that the stack trace really does hold
> references
> > to the objects in it, or just a listing of the class names and line
> numbers.
> > Also do exceptions hold the stack traces on a per-thread basis, as if
> they
> > are thread locals? Again I'm not sure, but if they do then multiple
> threads
> > throwing the same exception instance simultaneously will work happily.
> >
> > Anyway, I've refactored out the *ugly* code, so I don't have to puzzle
> about
> > these things.
> >
> > Rupert
> >
> > On 18/05/07, Robert Godfrey <ro...@gmail.com> wrote:
> > >
> > > Absolutely !
> > >
> > > On 18/05/07, John O'Hara <jo...@gmail.com> wrote:
> > > > Perhaps worth some comments to that effect, given it is a bit wierd.
> > > >
> > > > On 18/05/07, Robert Godfrey <ro...@gmail.com> wrote:
> > > > >
> > > > > No - its a single instance of the Exception. So there's no
> leak...
> > > > > and the stack is not interesting... and there's no thread safety
> > > > > issue
> > > > >
> > > > > The exception is used to flag a particular type of exception which
> > > > > leads to an AMQP error
> > > > > There's no need to have the stack trace retained.
> > > > >
> > > > > It's just like having enumerated error codes returned.
> > > > >
> > > > > It's *ugly* code, I'll grant you that :-)
> > > > >
> > > > >
> > > > > -- Rob
> > > > >
> > > > > On 18/05/07, Rupert Smith <ru...@googlemail.com> wrote:
> > > > > > Just found this shocker:
> > > > > >
> > > > > > private static final ExistingSubscriptionPreventsExclusive
> > > > > > EXISTING_SUBSCRIPTION =
> > > > > > new ExistingSubscriptionPreventsExclusive();
> > > > > >
> > > > > > Looks like a really bad idea as doesn't seem thread safe, as
> > > mutliple
> > > > > > threads raising these exceptions will overwrite each others
> stacks.
> > > I
> > > > > not
> > > > > > even sure that throw is thread safe in that respect? This could
> also
> > > > > cause
> > > > > > memory leaks, because objects refed by the static stack dump
> won't
> > > be
> > > > > > garbage collected.
> > > > > >
> > > > > > Rupert
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: More bad exception code...
Posted by Robert Godfrey <ro...@gmail.com>.
The stack trace is generated on Exception creation, not on throwing...
so it is completely safe. The stack trace in that exception would be
the stack at class instantiation.
This sort of trick is normally used where the exception is thrown
quite frequently in a semi- performance critical piece of code. I'm
not sure that was really an issue here... but be careful with this
sort of re-factoring (obviously there should hae been a comment about
why this was there)...
- Rob
On 18/05/07, Rupert Smith <ru...@googlemail.com> wrote:
> Well, I'm actually not sure that the stack trace really does hold references
> to the objects in it, or just a listing of the class names and line numbers.
> Also do exceptions hold the stack traces on a per-thread basis, as if they
> are thread locals? Again I'm not sure, but if they do then multiple threads
> throwing the same exception instance simultaneously will work happily.
>
> Anyway, I've refactored out the *ugly* code, so I don't have to puzzle about
> these things.
>
> Rupert
>
> On 18/05/07, Robert Godfrey <ro...@gmail.com> wrote:
> >
> > Absolutely !
> >
> > On 18/05/07, John O'Hara <jo...@gmail.com> wrote:
> > > Perhaps worth some comments to that effect, given it is a bit wierd.
> > >
> > > On 18/05/07, Robert Godfrey <ro...@gmail.com> wrote:
> > > >
> > > > No - its a single instance of the Exception. So there's no leak...
> > > > and the stack is not interesting... and there's no thread safety
> > > > issue
> > > >
> > > > The exception is used to flag a particular type of exception which
> > > > leads to an AMQP error
> > > > There's no need to have the stack trace retained.
> > > >
> > > > It's just like having enumerated error codes returned.
> > > >
> > > > It's *ugly* code, I'll grant you that :-)
> > > >
> > > >
> > > > -- Rob
> > > >
> > > > On 18/05/07, Rupert Smith <ru...@googlemail.com> wrote:
> > > > > Just found this shocker:
> > > > >
> > > > > private static final ExistingSubscriptionPreventsExclusive
> > > > > EXISTING_SUBSCRIPTION =
> > > > > new ExistingSubscriptionPreventsExclusive();
> > > > >
> > > > > Looks like a really bad idea as doesn't seem thread safe, as
> > mutliple
> > > > > threads raising these exceptions will overwrite each others stacks.
> > I
> > > > not
> > > > > even sure that throw is thread safe in that respect? This could also
> > > > cause
> > > > > memory leaks, because objects refed by the static stack dump won't
> > be
> > > > > garbage collected.
> > > > >
> > > > > Rupert
> > > > >
> > > >
> > >
> >
>
Re: More bad exception code...
Posted by Rupert Smith <ru...@googlemail.com>.
Well, I'm actually not sure that the stack trace really does hold references
to the objects in it, or just a listing of the class names and line numbers.
Also do exceptions hold the stack traces on a per-thread basis, as if they
are thread locals? Again I'm not sure, but if they do then multiple threads
throwing the same exception instance simultaneously will work happily.
Anyway, I've refactored out the *ugly* code, so I don't have to puzzle about
these things.
Rupert
On 18/05/07, Robert Godfrey <ro...@gmail.com> wrote:
>
> Absolutely !
>
> On 18/05/07, John O'Hara <jo...@gmail.com> wrote:
> > Perhaps worth some comments to that effect, given it is a bit wierd.
> >
> > On 18/05/07, Robert Godfrey <ro...@gmail.com> wrote:
> > >
> > > No - its a single instance of the Exception. So there's no leak...
> > > and the stack is not interesting... and there's no thread safety
> > > issue
> > >
> > > The exception is used to flag a particular type of exception which
> > > leads to an AMQP error
> > > There's no need to have the stack trace retained.
> > >
> > > It's just like having enumerated error codes returned.
> > >
> > > It's *ugly* code, I'll grant you that :-)
> > >
> > >
> > > -- Rob
> > >
> > > On 18/05/07, Rupert Smith <ru...@googlemail.com> wrote:
> > > > Just found this shocker:
> > > >
> > > > private static final ExistingSubscriptionPreventsExclusive
> > > > EXISTING_SUBSCRIPTION =
> > > > new ExistingSubscriptionPreventsExclusive();
> > > >
> > > > Looks like a really bad idea as doesn't seem thread safe, as
> mutliple
> > > > threads raising these exceptions will overwrite each others stacks.
> I
> > > not
> > > > even sure that throw is thread safe in that respect? This could also
> > > cause
> > > > memory leaks, because objects refed by the static stack dump won't
> be
> > > > garbage collected.
> > > >
> > > > Rupert
> > > >
> > >
> >
>
Re: More bad exception code...
Posted by Robert Godfrey <ro...@gmail.com>.
Absolutely !
On 18/05/07, John O'Hara <jo...@gmail.com> wrote:
> Perhaps worth some comments to that effect, given it is a bit wierd.
>
> On 18/05/07, Robert Godfrey <ro...@gmail.com> wrote:
> >
> > No - its a single instance of the Exception. So there's no leak...
> > and the stack is not interesting... and there's no thread safety
> > issue
> >
> > The exception is used to flag a particular type of exception which
> > leads to an AMQP error
> > There's no need to have the stack trace retained.
> >
> > It's just like having enumerated error codes returned.
> >
> > It's *ugly* code, I'll grant you that :-)
> >
> >
> > -- Rob
> >
> > On 18/05/07, Rupert Smith <ru...@googlemail.com> wrote:
> > > Just found this shocker:
> > >
> > > private static final ExistingSubscriptionPreventsExclusive
> > > EXISTING_SUBSCRIPTION =
> > > new ExistingSubscriptionPreventsExclusive();
> > >
> > > Looks like a really bad idea as doesn't seem thread safe, as mutliple
> > > threads raising these exceptions will overwrite each others stacks. I
> > not
> > > even sure that throw is thread safe in that respect? This could also
> > cause
> > > memory leaks, because objects refed by the static stack dump won't be
> > > garbage collected.
> > >
> > > Rupert
> > >
> >
>
Re: More bad exception code...
Posted by John O'Hara <jo...@gmail.com>.
Perhaps worth some comments to that effect, given it is a bit wierd.
On 18/05/07, Robert Godfrey <ro...@gmail.com> wrote:
>
> No - its a single instance of the Exception. So there's no leak...
> and the stack is not interesting... and there's no thread safety
> issue
>
> The exception is used to flag a particular type of exception which
> leads to an AMQP error
> There's no need to have the stack trace retained.
>
> It's just like having enumerated error codes returned.
>
> It's *ugly* code, I'll grant you that :-)
>
>
> -- Rob
>
> On 18/05/07, Rupert Smith <ru...@googlemail.com> wrote:
> > Just found this shocker:
> >
> > private static final ExistingSubscriptionPreventsExclusive
> > EXISTING_SUBSCRIPTION =
> > new ExistingSubscriptionPreventsExclusive();
> >
> > Looks like a really bad idea as doesn't seem thread safe, as mutliple
> > threads raising these exceptions will overwrite each others stacks. I
> not
> > even sure that throw is thread safe in that respect? This could also
> cause
> > memory leaks, because objects refed by the static stack dump won't be
> > garbage collected.
> >
> > Rupert
> >
>
Re: More bad exception code...
Posted by Robert Godfrey <ro...@gmail.com>.
No - its a single instance of the Exception. So there's no leak...
and the stack is not interesting... and there's no thread safety
issue
The exception is used to flag a particular type of exception which
leads to an AMQP error
There's no need to have the stack trace retained.
It's just like having enumerated error codes returned.
It's *ugly* code, I'll grant you that :-)
-- Rob
On 18/05/07, Rupert Smith <ru...@googlemail.com> wrote:
> Just found this shocker:
>
> private static final ExistingSubscriptionPreventsExclusive
> EXISTING_SUBSCRIPTION =
> new ExistingSubscriptionPreventsExclusive();
>
> Looks like a really bad idea as doesn't seem thread safe, as mutliple
> threads raising these exceptions will overwrite each others stacks. I not
> even sure that throw is thread safe in that respect? This could also cause
> memory leaks, because objects refed by the static stack dump won't be
> garbage collected.
>
> Rupert
>