|Anonymous | Login | Signup for a new account||2013-05-19 23:39 UTC|
|Main | My View | View Issues | Change Log | Roadmap | Summary | My Account|
|View Issue Details|
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0005972||opensim||[GRID] Other Service||public||2012-04-18 21:25||2012-05-05 02:48|
|Platform||Not paltform related||OS||Not OS related||OS Version||n/a|
|Product Version||master (dev code)|
|Target Version||Fixed in Version|
|Summary||0005972: In-world time is incorrect under certain setups|
|Description||- If I set my server (i mean the machine) on whatever timezone not using DST, viewer would show PST in anycase|
- If i set it to whatever timezone using DST, viewer would show PDT if applicable, PST otherwise (SL behaviour)
This is insane: we use a local setting for daylight savings, and a global hardcoded setting for the hour
This is not about being able to choose the timezone. This is about the current status of OpenSim: the timezone cannot be chosen, so the same time is supposed to appear everywhere.
Another side-effect is that even server respecting DST are likely to make the switch from PST to PDT at different time.
|Steps To Reproduce||Login with an account in OSGrid, check time (PST)|
Login with an account in another grid, configured on a "local-time" server, check time (PDT)
I installed a fresh server, and relaunched it several times after changing the time settings, I noticed the same results.
|Additional Information||DST is Daylight Saving Time. It's standard in north america, europe, and other countries.|
Second Life uses DST (it show PST time in winter, PDT time in summer)
OSGrid shows only PST
Imprudence allows to override SLTime and show UTC instead. However, setting it back to "PST/PDT" in Preferences > General produces the same behavior.
|Tags||No tags attached.|
|Git Revision or version number||0.7.3.1|
|Run Mode||Standalone (1 Region) , Standalone (Multiple Regions) , Grid (1 Region per Sim) , Grid (Multiple Regions per Sim)|
|Environment||Mono / Linux32, Mono / Linux64|
|Attached Files|| 009-d96aac4-DST_fix.patch [^] (1,801 bytes) 2012-04-19 02:03 [Show Content]
010-a46db36-DST_fix.patch [^] (1,796 bytes) 2012-04-24 07:57 [Show Content]
011-7118940-DST_fix.patch [^] (4,723 bytes) 2012-04-25 04:46 [Show Content]
013-d616944-DST_fix.patch [^] (7,576 bytes) 2012-04-26 16:24 [Show Content]
015-7cc062d-DST_fix.patch [^] (8,881 bytes) 2012-04-28 01:53 [Show Content]
Patch included to fix inconsistency. Has been tested on Mac and Linux, should be tested on Windows.
(Windows may or may not use a different time zone id)
Here is the tests I made:
- Set system time to GMT (or any no-daylight saving time zone)
- Launch the server and log inworld
- Check the displayed time. It should display "PDT"
- Shutdown server
- Set system time to Pacific Standard Time (or any daylight saving time zone, like Europe/paris)
- Launch the server and log inworld
- Check the displayed time. It should display "PDT"
Same test can be made on an un-patched version. In this case should display PST time on GMT system, and PDT time on Pacific Standard Time system
Looks like a good idea to me. Two small things
1) Could you use m_log.WarnFormat() rather than m_log.Warn() and concatenating strings? This is the approach used elsewhere in most of the code and should be used for new code.
2) Instead of calling IsDaylightSavingTime(DateTime.Now) ? "Y" : "N" twice, could you set a TimeZoneInfo local variable and call it just once after the exception catch instead, where the TimeZoneInfo is set to the found US/Pacific zone or the default system zone accordingly?
|Modified patch following your recommendations.|
|Thanks Olivier, Gudule. Applied as git master 0e3053e4.|
Unfortunately, as Melanie pointed out to me, not all Linux distributions (or Windows) have US/Pacific or the more arguably standard America/Los_Angeles.
Hence, this introduces an inconsistency that the server operator cannot control.
One option here is to introduce a config parameter in [LoginService] that specifies the name of the timezone to be used, which can also accept "local" as well as "America/Los_Angeles", etc. This parameter could be ; delimited to allow multiple timezones. In the future, such a list can then become the default but the operator still has the option of overriding this.
If the timezone information is passed in like this then in the future it can also be used to pass complete time information to any future viewer that supports this.
I wouldn't say it would "introduce" an inconsistency. It would probably not resolve "all" inconsistencies. Therefore is the fallback and the warning, as in this case, OpenSim would just behave as before.
I tested on 4 different linux flavors (debian, ubuntu, freebsd and macos) and the timezone format was recognized the same way. In these OS, America/Los_Angeles and US/Pacific are aliases.
The .Net documented value seems to be "Pacific Standard Time" (which linux don't recognize), but I know windows adopted also a lot of linux standards, that's why I suggested some tests under windows before adding another tests in the process.
Adding this value when someone confirms it is the good one would probably suppress the last risk of inconsistency.
I like the idea of using a list parameter, so the code would be ready when viewers accept the parameter.
However, I would rather pass
- the hardcoded value of Pacific DST under the current variable (DST)
- the custom complete timezone under a new variable (for example CustomTimeZone)
So using a custom value would not break the time for current viewers.
I thought again about it, and splitted the problem in two
1° Ensure the patches gives Pacific Time on most systems
2° Allow to override this settings.
SetDefaultValue() doesn't look like a convenient place to override a default value.
So, this new patch just fixes part 1°, including Windows time zone format (untested) and changing de var name to reflect they are default values.
Again, as it is now, the patch does not add any incoherency, it removes some, hopefully all.
However, it may be a good idea to let the grid admin the choice to keep the old system, for historic consistency. So I'll look again later (tomorrow?) how to implement something like "ForceLocalDST".
Rewrote the patch (013-d616944-DST_fix.patch), please disregards other ones
- DST check moved to LLLoginResponse instead of SetDefaultValue
- Windows-style timezone included
- New ini variable OverrideDST
- default: gives DST corresponding to Pacific Time (SL Time)
- off: disable DST (OSGrid Time)
- local: legacy behaviour
This way, grid owner have the choice to keep their current hour, while new installations would share one consistent system time.
I did not include the ability to choose a custom time zone, as it could mislead the admin to believe he can really choose in-world time-zone. This could create new inconsistencies compared to current situation.
If usage make appear that other strings are needed to fetch pacific time zone, I recommend updating this code.
If one would really choose another DST system (why?), he still can use the "local" feature, as he does now.
Comments on the latest patch, Gudule.
* The external timezones string should be in the external ini, even if it's also the internal default if this setting does not exist. This should replace "default". The string will not disappear since it would be in OpenSimDefaults.ini.
* Only America/Los_Angeles is required for Linux - I understand this is a more standard name than US/Pacific.
* The Linux distribution Melanie quoted to me as not supporting US/Pacific was CentOS.
* I would prefer the setting to be called DSTZone rather than OverrideDST. I think people can understand that this doesn't cover time. AFAIK, we don't bother to send a timezone string to the viewer, though in principle this would be possible if a viewer supported it.
* "local" must be supported since it's the current effective setting. This allows people to maintain it if they don't want to change external systems (e.g. events) that may rely on such a timezone setting, even though it's wrong wrt US daylight time.
* I think "none" is a better name than "off" now.
* There's no config section for Robust.ini.example
* Please do a null check for the IsDaylightSavingTime() rather than relying on catching an exception when dstTimeZone is null.
Thanks for your work on this Gudule. I know it might be annoying but this is the price one pays for trying to satisfy a bunch of existing use cases and users.
Thanks for the feedback.
First: a question, I would need an answer before going back to the code:
I tried several method to check if dstTimeZone was null, but they all failed and produced errors (empty not defined for timezoneinfo type, etc...). Can you tell me the right way to check if a TimeZoneInfo is null?
Now the other comments…
Sorry if I may appear touchy about this. The bug created a situation in which people believed their config was right, while it produced inconsistencies. I just don't want the fix to introduce a new additional way to create unwanted inconsistency.
No problem for the additional work. Time is a subject of debate in world as well as DST is a subject of debate in RL. Fixing the bug is also the opportunity to let users choose wether or not using it. Hence the "off/none" setting.
No problem to support "local" setting. Just a problem to recommend it. Most people won't realize that DST doesn't change on the same dates worldwide. It depends of the country. So that is the setting with which the admin would believe his grid is on the same time than some other grids, while it's time will differ twice a year during a couple of weeks. Which is not as easy to track as 6 months a year.
This is also why I really don't think it's pertinent to allow changing the tz search string in config files (given the "local" setting exists). But I will.
- OpenSimDefaults.ini: i thought about using this file, but this setting is only relevant to standalone and robust itself. Indeed, the whole LoginService section is absent from OpenSim*.ini. I followed previous usage for that and using main config file would imply moving the whole LoginService section.
- OK for America/Los_Angeles. I had made full tests with US/Pacific, and only some with America/Los_Angeles, but if you're sure it's ok, i'll keep only America/Los_Angeles. I guess this covers CentOS
- OK for DSTZone and "none"
- Oops. Forgot Robust-no-hg ini ;-)
* What I meant is that the later IsDaylightSavingTime() check didn't need a try catch. I see that for FindSystemTimeZoneById() you do have to do the exception catching.
* I will advocate having Pacific DST as the default, the local setting will be for people who need the previous ebehaviour.
* Yes, I made a mistake on OpenSimDefaults.ini - that doesn't apply in this situation, sorry. For standalone, this data can go in Standalone.ini (rather than StandaloneCommon,ini). For grid you do have to put it in Robust.ini/Robust.HG.ini at the moment - I think we will just have to go for this.
* I'm pretty sure about America/Los_Angeles. If I'm wrong I'll correct it later on.
Here we are. Hopefully ;-)
"Later IsDaylightSavingTime()" was also what I meant. I just tried again and it worked now. I must have missed something the first time.
Thanks to you too, Justin, for the time you took thinking about it and helping to make this fix more suitable for all of us.
Hi Gudule. I have now applied the latest patch as git master cccef2e5. Unfortunately, it wouldn't apply cleanly (I think you're based on a slightly older version of OpenSimulator) but rather than go another round of patch amendment I decided to fix up the small issues myself.
Thanks again for your work on this. Hopefully no further revisions will be needed but if they are I will make them directly myself.
|2012-04-18 21:25||GuduleLapointe||New Issue|
|2012-04-19 02:03||GuduleLapointe||File Added: 009-d96aac4-DST_fix.patch|
|2012-04-19 02:11||GuduleLapointe||Note Added: 0021270|
|2012-04-19 02:11||GuduleLapointe||Status||new => patch included|
|2012-04-24 00:30||justincc||Note Added: 0021282|
|2012-04-24 00:30||justincc||Assigned To||=> justincc|
|2012-04-24 00:30||justincc||Status||patch included => patch feedback|
|2012-04-24 07:57||GuduleLapointe||File Added: 010-a46db36-DST_fix.patch|
|2012-04-24 08:01||GuduleLapointe||Note Added: 0021284|
|2012-04-24 08:01||GuduleLapointe||Status||patch feedback => patch included|
|2012-04-24 19:32||justincc||Note Added: 0021288|
|2012-04-24 19:32||justincc||Status||patch included => resolved|
|2012-04-24 19:32||justincc||Resolution||open => fixed|
|2012-04-24 22:02||justincc||Status||resolved => new|
|2012-04-24 22:06||justincc||Note Added: 0021297|
|2012-04-24 22:53||GuduleLapointe||Note Added: 0021298|
|2012-04-25 04:46||GuduleLapointe||File Added: 011-7118940-DST_fix.patch|
|2012-04-25 04:47||GuduleLapointe||Note Added: 0021299|
|2012-04-25 04:47||GuduleLapointe||Status||new => patch included|
|2012-04-26 16:24||GuduleLapointe||File Added: 013-d616944-DST_fix.patch|
|2012-04-26 16:40||GuduleLapointe||Note Added: 0021310|
|2012-04-27 22:02||justincc||Note Added: 0021328|
|2012-04-27 22:05||justincc||Status||patch included => patch feedback|
|2012-04-27 23:19||GuduleLapointe||Note Added: 0021331|
|2012-04-27 23:24||justincc||Note Added: 0021332|
|2012-04-28 01:53||GuduleLapointe||File Added: 015-7cc062d-DST_fix.patch|
|2012-04-28 01:58||GuduleLapointe||Note Added: 0021334|
|2012-04-28 01:58||GuduleLapointe||Status||patch feedback => patch included|
|2012-05-05 02:48||justincc||Note Added: 0021371|
|2012-05-05 02:48||justincc||Status||patch included => resolved|
|Copyright © 2000 - 2012 MantisBT Group|