-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
RangeError: bignum too big to convert into `long' #4460
Comments
Wow, that's super weird. Given that it only happens sometimes, I thought it might be JIT kicking in sometimes. But then running with the JIT off (-X-C) still produces the error, so it's not JIT related. So given that there's very little outside the JIT that would be nondeterministic in JRuby, it seems like it must be some sort of concurrency problem, and I've marked the issue as such...but I have no idea where to look. It's pretty strange. |
After some experiments, I don't think it's concurrency related. First, I added some printf debugging (which sounds a lot better than System.err.println-debugging), the line with RubyInteger i = invokedynamic(context, begin, MethodNames.HASH).convertToInteger();
System.err.println("Value: " + i.getBigIntegerValue());
long v = i.getLongValue(); A few results of arrow up+return:
In other words: if the value of The implementation of def hash
return ([@addr, @mask_addr].hash << 1) | (ipv4? ? 0 : 1)
end If JRuby follows the model of Ruby for hashing, there is a random seed when the program starts. Paste |
And suddenly this cases reminds me that I still have to fix #4375. Well, not today |
I'm not exactly an expert here, but I guess the following change to def hash
return [@addr, @mask_addr].hash ^ (ipv4? ? 0 : 1)
end I'm not sure if the Downside of this all is that things have to be fixed in the stdlib of MRI |
Great research! Your patch might be good enough for 9.1.8.0, but I'm surprised that MRI doesn't have trouble with Bignum hashes too. Going to poke around a little bit. |
Ok, I believe I'd figured out the problem. MRI appears to allow
So this would seem to accommodate numbers from LONG_MIN to something like LONG_MIN * 2 -1 and allow our cases through. However, the following patch does not seem to be quite right. I hate bit math, especially when dealing with the lack of unsigned numerics in Java. diff --git a/core/src/main/java/org/jruby/RubyBignum.java b/core/src/main/java/org/jruby/RubyBignum.java
index c26e65b..bfc8ae0 100644
--- a/core/src/main/java/org/jruby/RubyBignum.java
+++ b/core/src/main/java/org/jruby/RubyBignum.java
@@ -158,12 +158,19 @@ public class RubyBignum extends RubyInteger {
*
*/
public static long big2long(RubyBignum value) {
- BigInteger big = value.getValue();
+ long num = big2ulong(value);
- if (big.compareTo(LONG_MIN) < 0 || big.compareTo(LONG_MAX) > 0) {
- throw value.getRuntime().newRangeError("bignum too big to convert into `long'");
+ if (value.value.signum() > 0) {
+ if (num <= Long.MAX_VALUE) {
+ return num;
+ }
+ } else {
+ if (num / 2 < Long.MAX_VALUE) {
+ return num;
+ }
}
- return big.longValue();
+
+ throw value.getRuntime().newRangeError("bignum too big to convert into `long'");
}
/** rb_big2ulong I'll take another stab at this tomorrow, but if you get to it before me I wouldn't complain. |
I should rephrase; changing our
|
For the record I'm mostly ok with the IPAddr patch, but I'd really like to fix our big2long logic properly. |
I'm going to go with the IPAddr patch for now. The spec failures above show how sensitive this big2long logic is to changes, and we shouldn't rush that into 9.1.8.0 at the last minute. We should undo the IPAddr patch after fixing this properly in 9.2. |
I have applied your temporary patch to ipaddr.rb. There's also a simpler workaround that doesn't involve stdlib, since this is simply a flaw in how our Range handles start or end hash resulting in a Bignum: class X
def <=>(other)
1
end
def hash
1<<63
end
end
p ((X.new)..1).hash We raise the error above while MRI handles this ok. |
Temporary patch to work around Range not supporting Bignum hashes.
Temporary patch to work around Range not supporting Bignum hashes.
It looks like the IPAddr class has been moved from the core to a gem (see https://github.com/ruby/ipaddr), which means the temporary changes will be ineffective in a near future. |
@herwinw The new gemified stdlib will be for Ruby 2.5+, and we will work with them to get JRuby-specific versions released. Until then, though, we'll still ship our own copy of stdlib. We will also work to get the hack out of JRuby. |
Link #4875 |
The secret missing logic was in |
This reverts commit a985aac.
This reverts commit 61a6b24.
Temporary patch to work around Range not supporting Bignum hashes.
This reverts commit 61a6b24.
Temporary patch to work around Range not supporting Bignum hashes.
This reverts commit 61a6b24.
The following script
ocasionally generates the following exception:
Related part of the code:
Running
r.begin.hash
does not appear to break (it is hard to test however, since the exception doesn't happen every time)Environment
Provide at least:
JRuby version (
jruby -v
) and command line (flags, JRUBY_OPTS, etc)jruby 9.1.7.0 (2.3.1) 2017-01-11 68056ae OpenJDK Server VM 25.111-b14 on 1.8.0_111-8u111-b14-2~bpo8+1-b14 +jit [linux-i386] (installed via rbenv, no extra command line options)
Operating system and platform (e.g.
uname -a
)Linux 3.16.0-4-amd64 break script engine #1 SMP Debian 3.16.36-1+deb8u2 (2016-10-19) x86_64 GNU/Linux (but running a 32-bit userspace)
The text was updated successfully, but these errors were encountered: