String "" in Jetty 8 vs Jetty 9

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

String "" in Jetty 8 vs Jetty 9

Kapil
Hi,

We were using Jetty 8 and now trying to upgrade on Jetty 9. The below piece of code has different results in both versions of Jetty.

if (a != "") 

Where a is string variable. 

I understand the code is wrong, we should always use a.equals for comparison. But we have it at so many places, but not sure why Jetty 8 and Jetty 9 have different results for it.

Please let me know.

Thanks


_______________________________________________
jetty-users mailing list
[hidden email]
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/jetty-users
Reply | Threaded
Open this post in threaded view
|

Re: String "" in Jetty 8 vs Jetty 9

Kapil
This is for request.getParameter

Thanks

On Fri, Jan 4, 2019 at 12:30 PM kapil gupta <[hidden email]> wrote:
Hi,

We were using Jetty 8 and now trying to upgrade on Jetty 9. The below piece of code has different results in both versions of Jetty.

if (a != "") 

Where a is string variable. 

I understand the code is wrong, we should always use a.equals for comparison. But we have it at so many places, but not sure why Jetty 8 and Jetty 9 have different results for it.

Please let me know.

Thanks


_______________________________________________
jetty-users mailing list
[hidden email]
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/jetty-users
Reply | Threaded
Open this post in threaded view
|

Re: String "" in Jetty 8 vs Jetty 9

Mark Mielke
On Fri, Jan 4, 2019 at 2:27 AM kapil gupta <[hidden email]> wrote:
This is for request.getParameter

On Fri, Jan 4, 2019 at 12:30 PM kapil gupta <[hidden email]> wrote:
We were using Jetty 8 and now trying to upgrade on Jetty 9. The below piece of code has different results in both versions of Jetty.

if (a != "") 

Where a is string variable. 

I understand the code is wrong, we should always use a.equals for comparison. But we have it at so many places, but not sure why Jetty 8 and Jetty 9 have different results for it.


If you think the question above is valid, then the code is so much more wrong than you are recognizing. :-)

The "" generates a constant string that is at least in principle allocated in the *calling* class. This means it is part of your class. Within your class, it is *possible* that the compiler will do de-duplication of the constant strings, and emit just one constant string representing "" for your whole class, which means that it is *possible* that "" will have the same identity anywhere in your class. I emphasize *possible*, as there is no language level guarantee for this. This is an invalid assumption that should never make it past code review.

Any "" that gets generated from other classes, would either start from a constant string defined in another class (with a different identity!), or start from a char[0] which will always have a new identity. There is almost zero way your code could work in Jetty 8.

I say "almost zero", because there is a way... some Java runtimes may de-duplicate constant strings across classes, or during runtime garbage collection. This may also be triggered manually by using String.intern()  ( https://docs.oracle.com/javase/7/docs/api/java/lang/String.html#intern() ). In such cases, it's possible that your code might work, sometimes.

"Might work sometimes" dependent upon a runtime feature that was probably not understood at the time it was accidentally selected, is a very bad way to write code.

You need to fix your code.

--
Mark Mielke <[hidden email]>


_______________________________________________
jetty-users mailing list
[hidden email]
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/jetty-users
Reply | Threaded
Open this post in threaded view
|

Re: String "" in Jetty 8 vs Jetty 9

Joakim Erdfelt-8
In reply to this post by Kapil
The fact that it worked for you in Jetty 8 is just luck.
You accidentally found a way on your JVM to have that work for you in a way that seemed reliable to you.

As Mark Mielke pointed out, that code is wrong, and shouldn't have worked in the first place, even on Jetty 8.

Also note that getParameter(String keyname) can return 3 different states:

1. The value of the keyname as a String
2. Empty String (indicating the keyname exists, but without a value, aka "?foo" )
3. null (indicating the keyname does not exist)

Joakim Erdfelt / [hidden email]


On Fri, Jan 4, 2019 at 1:27 AM kapil gupta <[hidden email]> wrote:
This is for request.getParameter

Thanks

On Fri, Jan 4, 2019 at 12:30 PM kapil gupta <[hidden email]> wrote:
Hi,

We were using Jetty 8 and now trying to upgrade on Jetty 9. The below piece of code has different results in both versions of Jetty.

if (a != "") 

Where a is string variable. 

I understand the code is wrong, we should always use a.equals for comparison. But we have it at so many places, but not sure why Jetty 8 and Jetty 9 have different results for it.

Please let me know.

Thanks

_______________________________________________
jetty-users mailing list
[hidden email]
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/jetty-users

_______________________________________________
jetty-users mailing list
[hidden email]
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/jetty-users
Reply | Threaded
Open this post in threaded view
|

Re: String "" in Jetty 8 vs Jetty 9

Mark Mielke
In reply to this post by Mark Mielke
Update to my explanation. String.intern() does refer to this portion of the Java Language Specification:


"Moreover, a string literal always refers to the same instance of class String. This is because string literals - or, more generally, strings that are the values of constant expressions (§15.28) - are "interned" so as to share unique instances, using the method String.intern."

So, there is a language specification support for the understanding that the constant string "" from one class, will be both String.equals() and == another constant string "" in the same Java runtime.

However, there is nothing in the Jetty documentation that specifies that the return result of .getParameter() will be a constant string when it is empty. So, any code that assumes this is susceptible to breakage with any change to the Jetty implementation. Even a very small indirect change could influence whether or not it *happens* to return a constant string or not. I stand firmly by my stance that such code should never have passed code review. That something happens to work, isn't sufficient quality code for deployment, and it certainly isn't a good reason to restore the old behaviour.

Despite it being a really bad assumption... you piqued my curiosity as to exactly how it might have happened that the behaviour changed.

I think this was the commit that "broke" you:


It looks like the code previously had a few hard coded locations where it would have a fast path for zero string ( l==0 ? "" : ... ), a fast path for strings that had no decoding requirements, and a slow path for decoding. Greg replaced several of these with generic code that would would do the right thing every time in one common place. The replacement code probably does not special case "if empty string, return constant empty string."

Probably, the behaviour could be restored in one place. But, probably this would be a very bad precedent to set, as it basically agrees that users can expect constant literals to be returned everywhere else they might currently be returned, and this could lead to enabling other bad code to continue functioning instead of being corrected. I'm tempted to go the opposite way here - purposefully avoiding the use of String.intern() or constants, specifically to avoid the case of people relying on undocumented behaviour. Prevent people from making a mistake in the first place.


On Fri, Jan 4, 2019 at 8:00 AM Mark Mielke <[hidden email]> wrote:
On Fri, Jan 4, 2019 at 2:27 AM kapil gupta <[hidden email]> wrote:
This is for request.getParameter

On Fri, Jan 4, 2019 at 12:30 PM kapil gupta <[hidden email]> wrote:
We were using Jetty 8 and now trying to upgrade on Jetty 9. The below piece of code has different results in both versions of Jetty.

if (a != "") 

Where a is string variable. 

I understand the code is wrong, we should always use a.equals for comparison. But we have it at so many places, but not sure why Jetty 8 and Jetty 9 have different results for it.


If you think the question above is valid, then the code is so much more wrong than you are recognizing. :-)

The "" generates a constant string that is at least in principle allocated in the *calling* class. This means it is part of your class. Within your class, it is *possible* that the compiler will do de-duplication of the constant strings, and emit just one constant string representing "" for your whole class, which means that it is *possible* that "" will have the same identity anywhere in your class. I emphasize *possible*, as there is no language level guarantee for this. This is an invalid assumption that should never make it past code review.

Any "" that gets generated from other classes, would either start from a constant string defined in another class (with a different identity!), or start from a char[0] which will always have a new identity. There is almost zero way your code could work in Jetty 8.

I say "almost zero", because there is a way... some Java runtimes may de-duplicate constant strings across classes, or during runtime garbage collection. This may also be triggered manually by using String.intern()  ( https://docs.oracle.com/javase/7/docs/api/java/lang/String.html#intern() ). In such cases, it's possible that your code might work, sometimes.

"Might work sometimes" dependent upon a runtime feature that was probably not understood at the time it was accidentally selected, is a very bad way to write code.

You need to fix your code.

--
Mark Mielke <[hidden email]>



--
Mark Mielke <[hidden email]>


_______________________________________________
jetty-users mailing list
[hidden email]
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/jetty-users
Reply | Threaded
Open this post in threaded view
|

Re: String "" in Jetty 8 vs Jetty 9

Kapil
In reply to this post by Mark Mielke
I think the issue is with javax-servlet-api, as it got changed in jetty 9. The way the string was handled is different now, yes I do agree the application code is wrong.

On Sat, Jan 5, 2019 at 8:33 AM Mark Mielke <[hidden email]> wrote:
Boxbe Mark Mielke ([hidden email]) added themselves to your Guest List | Remove them | Block them
Update to my explanation. String.intern() does refer to this portion of the Java Language Specification:


"Moreover, a string literal always refers to the same instance of class String. This is because string literals - or, more generally, strings that are the values of constant expressions (§15.28) - are "interned" so as to share unique instances, using the method String.intern."

So, there is a language specification support for the understanding that the constant string "" from one class, will be both String.equals() and == another constant string "" in the same Java runtime.

However, there is nothing in the Jetty documentation that specifies that the return result of .getParameter() will be a constant string when it is empty. So, any code that assumes this is susceptible to breakage with any change to the Jetty implementation. Even a very small indirect change could influence whether or not it *happens* to return a constant string or not. I stand firmly by my stance that such code should never have passed code review. That something happens to work, isn't sufficient quality code for deployment, and it certainly isn't a good reason to restore the old behaviour.

Despite it being a really bad assumption... you piqued my curiosity as to exactly how it might have happened that the behaviour changed.

I think this was the commit that "broke" you:


It looks like the code previously had a few hard coded locations where it would have a fast path for zero string ( l==0 ? "" : ... ), a fast path for strings that had no decoding requirements, and a slow path for decoding. Greg replaced several of these with generic code that would would do the right thing every time in one common place. The replacement code probably does not special case "if empty string, return constant empty string."

Probably, the behaviour could be restored in one place. But, probably this would be a very bad precedent to set, as it basically agrees that users can expect constant literals to be returned everywhere else they might currently be returned, and this could lead to enabling other bad code to continue functioning instead of being corrected. I'm tempted to go the opposite way here - purposefully avoiding the use of String.intern() or constants, specifically to avoid the case of people relying on undocumented behaviour. Prevent people from making a mistake in the first place.


On Fri, Jan 4, 2019 at 8:00 AM Mark Mielke <[hidden email]> wrote:
On Fri, Jan 4, 2019 at 2:27 AM kapil gupta <[hidden email]> wrote:
This is for request.getParameter

On Fri, Jan 4, 2019 at 12:30 PM kapil gupta <[hidden email]> wrote:
We were using Jetty 8 and now trying to upgrade on Jetty 9. The below piece of code has different results in both versions of Jetty.

if (a != "") 

Where a is string variable. 

I understand the code is wrong, we should always use a.equals for comparison. But we have it at so many places, but not sure why Jetty 8 and Jetty 9 have different results for it.


If you think the question above is valid, then the code is so much more wrong than you are recognizing. :-)

The "" generates a constant string that is at least in principle allocated in the *calling* class. This means it is part of your class. Within your class, it is *possible* that the compiler will do de-duplication of the constant strings, and emit just one constant string representing "" for your whole class, which means that it is *possible* that "" will have the same identity anywhere in your class. I emphasize *possible*, as there is no language level guarantee for this. This is an invalid assumption that should never make it past code review.

Any "" that gets generated from other classes, would either start from a constant string defined in another class (with a different identity!), or start from a char[0] which will always have a new identity. There is almost zero way your code could work in Jetty 8.

I say "almost zero", because there is a way... some Java runtimes may de-duplicate constant strings across classes, or during runtime garbage collection. This may also be triggered manually by using String.intern()  ( https://docs.oracle.com/javase/7/docs/api/java/lang/String.html#intern() ). In such cases, it's possible that your code might work, sometimes.

"Might work sometimes" dependent upon a runtime feature that was probably not understood at the time it was accidentally selected, is a very bad way to write code.

You need to fix your code.

--
Mark Mielke <[hidden email]>



--
Mark Mielke <[hidden email]>


_______________________________________________
jetty-users mailing list
[hidden email]
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/jetty-users
Reply | Threaded
Open this post in threaded view
|

Re: String "" in Jetty 8 vs Jetty 9

Joakim Erdfelt-8
The `javax-servlet-api` is not an artifact that Jetty builds / deploys.
That is the Standard JEE API jar deployed by the Servlet Expert Group from the old java.net (Oracle recently donated all of JEE to the Eclipse Foundation, there are no public releases from this effort yet)
Jetty just uses it.

Joakim Erdfelt / [hidden email]


On Tue, Jan 8, 2019 at 4:10 AM kapil gupta <[hidden email]> wrote:
I think the issue is with javax-servlet-api, as it got changed in jetty 9. The way the string was handled is different now, yes I do agree the application code is wrong.

On Sat, Jan 5, 2019 at 8:33 AM Mark Mielke <[hidden email]> wrote:
Boxbe Mark Mielke ([hidden email]) added themselves to your Guest List | Remove them | Block them
Update to my explanation. String.intern() does refer to this portion of the Java Language Specification:


"Moreover, a string literal always refers to the same instance of class String. This is because string literals - or, more generally, strings that are the values of constant expressions (§15.28) - are "interned" so as to share unique instances, using the method String.intern."

So, there is a language specification support for the understanding that the constant string "" from one class, will be both String.equals() and == another constant string "" in the same Java runtime.

However, there is nothing in the Jetty documentation that specifies that the return result of .getParameter() will be a constant string when it is empty. So, any code that assumes this is susceptible to breakage with any change to the Jetty implementation. Even a very small indirect change could influence whether or not it *happens* to return a constant string or not. I stand firmly by my stance that such code should never have passed code review. That something happens to work, isn't sufficient quality code for deployment, and it certainly isn't a good reason to restore the old behaviour.

Despite it being a really bad assumption... you piqued my curiosity as to exactly how it might have happened that the behaviour changed.

I think this was the commit that "broke" you:


It looks like the code previously had a few hard coded locations where it would have a fast path for zero string ( l==0 ? "" : ... ), a fast path for strings that had no decoding requirements, and a slow path for decoding. Greg replaced several of these with generic code that would would do the right thing every time in one common place. The replacement code probably does not special case "if empty string, return constant empty string."

Probably, the behaviour could be restored in one place. But, probably this would be a very bad precedent to set, as it basically agrees that users can expect constant literals to be returned everywhere else they might currently be returned, and this could lead to enabling other bad code to continue functioning instead of being corrected. I'm tempted to go the opposite way here - purposefully avoiding the use of String.intern() or constants, specifically to avoid the case of people relying on undocumented behaviour. Prevent people from making a mistake in the first place.


On Fri, Jan 4, 2019 at 8:00 AM Mark Mielke <[hidden email]> wrote:
On Fri, Jan 4, 2019 at 2:27 AM kapil gupta <[hidden email]> wrote:
This is for request.getParameter

On Fri, Jan 4, 2019 at 12:30 PM kapil gupta <[hidden email]> wrote:
We were using Jetty 8 and now trying to upgrade on Jetty 9. The below piece of code has different results in both versions of Jetty.

if (a != "") 

Where a is string variable. 

I understand the code is wrong, we should always use a.equals for comparison. But we have it at so many places, but not sure why Jetty 8 and Jetty 9 have different results for it.


If you think the question above is valid, then the code is so much more wrong than you are recognizing. :-)

The "" generates a constant string that is at least in principle allocated in the *calling* class. This means it is part of your class. Within your class, it is *possible* that the compiler will do de-duplication of the constant strings, and emit just one constant string representing "" for your whole class, which means that it is *possible* that "" will have the same identity anywhere in your class. I emphasize *possible*, as there is no language level guarantee for this. This is an invalid assumption that should never make it past code review.

Any "" that gets generated from other classes, would either start from a constant string defined in another class (with a different identity!), or start from a char[0] which will always have a new identity. There is almost zero way your code could work in Jetty 8.

I say "almost zero", because there is a way... some Java runtimes may de-duplicate constant strings across classes, or during runtime garbage collection. This may also be triggered manually by using String.intern()  ( https://docs.oracle.com/javase/7/docs/api/java/lang/String.html#intern() ). In such cases, it's possible that your code might work, sometimes.

"Might work sometimes" dependent upon a runtime feature that was probably not understood at the time it was accidentally selected, is a very bad way to write code.

You need to fix your code.

--
Mark Mielke <[hidden email]>



--
Mark Mielke <[hidden email]>

_______________________________________________
jetty-users mailing list
[hidden email]
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/jetty-users

_______________________________________________
jetty-users mailing list
[hidden email]
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/jetty-users
Reply | Threaded
Open this post in threaded view
|

Re: String "" in Jetty 8 vs Jetty 9

Mark Mielke
In reply to this post by Kapil


On Tue, Jan 8, 2019, 5:10 AM kapil gupta <[hidden email] wrote:
I think the issue is with javax-servlet-api, as it got changed in jetty 9. The way the string was handled is different now, yes I do agree the application code is wrong.

Read my explanation and read the commit I referenced. It should make sense then. This is not an api question. That api has nothing to do with this implementation artifact that you noticed. You noticed something that happened to work. You asked what changed. I provided both. 



On Sat, Jan 5, 2019 at 8:33 AM Mark Mielke <[hidden email]> wrote:
Boxbe Mark Mielke ([hidden email]) added themselves to your Guest List | Remove them | Block them
Update to my explanation. String.intern() does refer to this portion of the Java Language Specification:


"Moreover, a string literal always refers to the same instance of class String. This is because string literals - or, more generally, strings that are the values of constant expressions (§15.28) - are "interned" so as to share unique instances, using the method String.intern."

So, there is a language specification support for the understanding that the constant string "" from one class, will be both String.equals() and == another constant string "" in the same Java runtime.

However, there is nothing in the Jetty documentation that specifies that the return result of .getParameter() will be a constant string when it is empty. So, any code that assumes this is susceptible to breakage with any change to the Jetty implementation. Even a very small indirect change could influence whether or not it *happens* to return a constant string or not. I stand firmly by my stance that such code should never have passed code review. That something happens to work, isn't sufficient quality code for deployment, and it certainly isn't a good reason to restore the old behaviour.

Despite it being a really bad assumption... you piqued my curiosity as to exactly how it might have happened that the behaviour changed.

I think this was the commit that "broke" you:


It looks like the code previously had a few hard coded locations where it would have a fast path for zero string ( l==0 ? "" : ... ), a fast path for strings that had no decoding requirements, and a slow path for decoding. Greg replaced several of these with generic code that would would do the right thing every time in one common place. The replacement code probably does not special case "if empty string, return constant empty string."

Probably, the behaviour could be restored in one place. But, probably this would be a very bad precedent to set, as it basically agrees that users can expect constant literals to be returned everywhere else they might currently be returned, and this could lead to enabling other bad code to continue functioning instead of being corrected. I'm tempted to go the opposite way here - purposefully avoiding the use of String.intern() or constants, specifically to avoid the case of people relying on undocumented behaviour. Prevent people from making a mistake in the first place.


On Fri, Jan 4, 2019 at 8:00 AM Mark Mielke <[hidden email]> wrote:
On Fri, Jan 4, 2019 at 2:27 AM kapil gupta <[hidden email]> wrote:
This is for request.getParameter

On Fri, Jan 4, 2019 at 12:30 PM kapil gupta <[hidden email]> wrote:
We were using Jetty 8 and now trying to upgrade on Jetty 9. The below piece of code has different results in both versions of Jetty.

if (a != "") 

Where a is string variable. 

I understand the code is wrong, we should always use a.equals for comparison. But we have it at so many places, but not sure why Jetty 8 and Jetty 9 have different results for it.


If you think the question above is valid, then the code is so much more wrong than you are recognizing. :-)

The "" generates a constant string that is at least in principle allocated in the *calling* class. This means it is part of your class. Within your class, it is *possible* that the compiler will do de-duplication of the constant strings, and emit just one constant string representing "" for your whole class, which means that it is *possible* that "" will have the same identity anywhere in your class. I emphasize *possible*, as there is no language level guarantee for this. This is an invalid assumption that should never make it past code review.

Any "" that gets generated from other classes, would either start from a constant string defined in another class (with a different identity!), or start from a char[0] which will always have a new identity. There is almost zero way your code could work in Jetty 8.

I say "almost zero", because there is a way... some Java runtimes may de-duplicate constant strings across classes, or during runtime garbage collection. This may also be triggered manually by using String.intern()  ( https://docs.oracle.com/javase/7/docs/api/java/lang/String.html#intern() ). In such cases, it's possible that your code might work, sometimes.

"Might work sometimes" dependent upon a runtime feature that was probably not understood at the time it was accidentally selected, is a very bad way to write code.

You need to fix your code.

--
Mark Mielke <[hidden email]>



--
Mark Mielke <[hidden email]>


_______________________________________________
jetty-users mailing list
[hidden email]
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/jetty-users