Skip to content
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

Time marshaling is incorrect #957

Closed
crawlik opened this Issue Aug 15, 2013 · 11 comments

Comments

Projects
None yet
6 participants
@crawlik
Copy link

commented Aug 15, 2013

Consider this snippet

$ jruby --1.9 /home/alex/.rvm/rubies/jruby-1.7.4/bin/irb
jruby-1.7.4 :001 > p Time.now.zone; p Marshal.load(Marshal.dump(Time.now)).zone
"CDT"
"GMT+5"
 => "GMT+5" 

Looks like jRuby in 1.9 mode doesn't marshal/unmarshal time correctly. Weird thing happens with timezone. I am in CDT and should have GMT-5.

Is it known problem? Is there a way to workaround it?

JRuby 1.8 looks ok

$ jruby --1.8 /home/alex/.rvm/rubies/jruby-1.7.4/bin/irb
jruby-1.7.4 :001 > require 'time'; tr = Time.now; tr == Marshal.load(Marshal.dump(tr))
 => true 
jruby-1.7.4 :002 > p Time.now.zone; p Marshal.load(Marshal.dump(Time.now)).zone
"CDT"
"CDT"
 => nil 

MRI 1.9.3 is broken too

ruby 1.9.3p392 (2013-02-22 revision 39386) [x86_64-linux]
irb(main):001:0> p Time.now.zone; p Marshal.load(Marshal.dump(Time.now)).zone
"CDT"
nil
=> nil

MRI 2.0 looks correct

ruby 2.0.0p247 (2013-06-27 revision 41674) [x86_64-linux]
2.0.0p247 :001 > p Time.now.zone; p Marshal.load(Marshal.dump(Time.now)).zone
"CDT"
"CDT"
 => "CDT" 
# original == to marshaled/unmarshalled
2.0.0p247 :002 > require 'time'; tr = Time.now; tr == Marshal.load(Marshal.dump(tr))
 => true 
@crawlik

This comment has been minimized.

Copy link
Author

commented Aug 15, 2013

Here is one more observation for JRuby 1.7.4. Original value seems to be equal to unmarshaled one. Looks zone is ignored in this case.

jruby-1.7.4 :003 > tr = Time.now; tr == Marshal.load(Marshal.dump(tr))
 => true 
@eregon

This comment has been minimized.

Copy link
Member

commented Sep 22, 2013

Seems a problem from Time#zone:

> t=Time.now
=> 2013-09-22 18:55:26 +0200
> s=Marshal.dump t
=> "\x04\bIu:\tTime\r\xD0b\x1C\x80 \xB9\xA3\xDD\a:\voffseti\x02 \x1C:\rsubmicro\"\x06\x00"
> r=Marshal.load s
=> 2013-09-22 18:55:26 +0200

> t.zone
=> "CEST"
> r.zone
=> "GMT-2"

> t.gmt_offset
=> 7200
> r.gmt_offset
=> 7200

I have two questions about the implem:

  • Is there any easy way to test the zone/offset map of JRuby and MRI?
  • The marshaling seems very complex, would it not be much simpler and efficient to dump a specific JRuby format? Is it required to be compatible at dumped binary marshal data with MRI?
@enebo

This comment has been minimized.

Copy link
Member

commented Sep 22, 2013

We have many users who do dev mode stuff in MRI and production stuff using JRuby. We also have people who run one service using MRI and nother with JRuby. Marshal binary compatibility can come into play in both of these scenarios so we should be binary compatible.

@eregon

This comment has been minimized.

Copy link
Member

commented Sep 22, 2013

@enebo @headius told me the same :)

@BanzaiMan In 8795981 should not the ternary condition have clauses in the other order? (char sign = minus_p ? '-' : '+';)

@BanzaiMan

This comment has been minimized.

Copy link
Member

commented Sep 23, 2013

@eregon Sign has to be inverted for Java's internal representation when you go between Java and Ruby times.

@moofish32

This comment has been minimized.

Copy link

commented Nov 7, 2014

@eregon Any updates on this issue? I'm up to 1.7.16 and just discovered this little secret. Not finding many work arounds. I did consider forcing to UTC before dumping the data, but that just feels hackish.

I actually think that I am not experiencing an issue with JRuby. I think this Rails issue may be more to the point
rails/rails#14442

JRuby console output:

irb(main):003:0> Marshal.load(Marshal.dump(a))
=> 2014-11-08 06:38:13 +0600
irb(main):004:0> a = Time.now
=> 2014-11-08 06:42:31 +0600
irb(main):005:0> Marshal.load(Marshal.dump(a))
=> 2014-11-08 06:42:31 +0600

Rails Console (JRuby) output:

Loading development environment (Rails 4.0.5)
irb(main):002:0> a = Time.now
=> 2014-11-08 06:43:34 +0600
irb(main):003:0> Marshal.load(Marshal.dump(a))
=> 2014-11-08 06:43:34 UTC
irb(main):004:0>
@eregon

This comment has been minimized.

Copy link
Member

commented Nov 8, 2014

@moofish32 I am not the author of Time marshalling in JRuby, so I might be of little help concerning the original code. I will try to have a look at what goes wrong in Rails though.

@eregon

This comment has been minimized.

Copy link
Member

commented Nov 8, 2014

@moofish32 Does the test in the Time marshalling patch,

Time.local(2010).zone != Marshal.load(Marshal.dump(Time.local(2010))).zone

fails (returns true) at you?

It seems fixed on JRuby master, which version are you using?

@eregon

This comment has been minimized.

Copy link
Member

commented Nov 8, 2014

Bad memories, it seems I did indeed fix it in 64b0cb7 but it seems it did not make it to jruby-1_7.

eregon added a commit that referenced this issue Nov 8, 2014

Rework TZ parsing and marshal the Time zone
* Much less duplication for HHMM parsing.
* Stricter and clearer methods for getting a DateTimeZone.
* Fixes incorrect types for offset.
* Fixes #1837 and #957.

Conflicts:
	core/src/main/java/org/jruby/RubyTime.java
	test/mri/excludes/TestTime.rb
@eregon

This comment has been minimized.

Copy link
Member

commented Nov 8, 2014

Fixed on the 1.7 branch in 24a70ed.

Best workaround for released 1.7 versions might be to avoid that active_support patching since it looks very fragile.

@headius

This comment has been minimized.

Copy link
Member

commented Nov 8, 2014

Thanks @eregon!

@headius headius closed this Nov 8, 2014

@headius headius added this to the JRuby 1.7.17 milestone Nov 8, 2014

yousuketto added a commit to yousuketto/jruby that referenced this issue Nov 22, 2014

Rework TZ parsing and marshal the Time zone
* Much less duplication for HHMM parsing.
* Stricter and clearer methods for getting a DateTimeZone.
* Fixes incorrect types for offset.
* Fixes jruby#1837 and jruby#957.

Conflicts:
	core/src/main/java/org/jruby/RubyTime.java
	test/mri/excludes/TestTime.rb

esparta added a commit to esparta/jruby that referenced this issue Feb 13, 2018

Strip punctuation from CSV headers in symbol converter.
  Patch by @cllns. [Fix jrubyGH-957]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@58744 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Signed-off-by: Espartaco Palma <esparta@gmail.com>

esparta added a commit to esparta/jruby that referenced this issue Feb 13, 2018

Strip punctuation from CSV headers in symbol converter.
  Patch by @cllns. [Fix jrubyGH-957]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@58744 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Signed-off-by: Espartaco Palma <esparta@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.