Port System.Timers.Timer to netstandard1.7 #11760
Conversation
| } | ||
|
|
||
| ElapsedEventArgs elapsedEventArgs = new ElapsedEventArgs(DateTime.UtcNow.ToFileTime()); | ||
| try |
There was a problem hiding this comment.
This implementation is changed from Desktop. Desktop implementation here is pinvoking to win32 apis, replaced with managed equivalent for cross-plat, since these are already implemented in coreclr pal. Measured the accuracy of the pinvoke and the managed apis here, got similar results.
There was a problem hiding this comment.
int stop = 0;
while (stop < 10)
{
FILE_TIME filetime = new FILE_TIME();
GetSystemTimeAsFileTime(ref filetime);
long d1 = (long)((((ulong)filetime.ftTimeHigh) << 32) | (((ulong)filetime.ftTimeLow) & 0xffffffff));
long ft = DateTime.UtcNow.ToFileTime();
Console.WriteLine("native: {0}", DateTime.FromFileTime(d1).Millisecond);
Console.WriteLine("manage: {0}", DateTime.FromFileTime(ft).Millisecond);
Thread.Sleep(1);
stop++;
}resulted in
native: 80
manage: 80
native: 86
manage: 86
native: 88
manage: 88
native: 90
manage: 90
native: 92
manage: 92
native: 94
manage: 94
native: 96
manage: 96
native: 98
manage: 98
native: 100
manage: 100
native: 102
manage: 102
There was a problem hiding this comment.
This seems reasonable to me.
| { | ||
| CountdownEvent cde = new CountdownEvent(1); | ||
| int result = 0; | ||
| _timer = new TestTimer(100); |
There was a problem hiding this comment.
This could use as well be 1ms as 100ms I guess
| "debian.8-x64", | ||
| "rhel.7-x64", | ||
| "ubuntu.14.04-x64", | ||
| "ubuntu.16.04-x64", |
There was a problem hiding this comment.
Please include ubuntu.16.10-x64 and opensuse.42.1-x64 here
| "netstandard1.7": { | ||
| "imports": [ | ||
| "dotnet5.1" | ||
| "dotnet5.8" |
There was a problem hiding this comment.
Is this really necessary still?
How come? |
| { | ||
| public class ElapsedEventArgs : EventArgs | ||
| { | ||
| private DateTime _signalTime; |
| protected virtual bool CanRaiseEvents { get { return default(bool); } } | ||
| protected bool DesignMode { get { return default(bool); } } | ||
| public virtual ISite Site { get { return default(ISite); } set { } } | ||
|
|
There was a problem hiding this comment.
Doesn't Component have a Disposed event?
| public partial class Component : IDisposable | ||
| { | ||
| public Component() { } | ||
| public void Dispose() { } |
There was a problem hiding this comment.
Dispose should invoke Dispose(true) and suppress finalization.
| protected virtual void Dispose(bool disposing) { } | ||
| ~Component() { } | ||
| protected virtual object GetService(Type service) { return default(object); } | ||
| public override string ToString() { return default(string); } |
There was a problem hiding this comment.
ToString() should return base.ToString().
| protected virtual object GetService(Type service) { return default(object); } | ||
| public override string ToString() { return default(string); } | ||
| protected virtual bool CanRaiseEvents { get { return default(bool); } } | ||
| protected bool DesignMode { get { return default(bool); } } |
There was a problem hiding this comment.
Nit: I realize it's the same thing, but this should probably be return false;
| ~Component() { } | ||
| protected virtual object GetService(Type service) { return default(object); } | ||
| public override string ToString() { return default(string); } | ||
| protected virtual bool CanRaiseEvents { get { return default(bool); } } |
There was a problem hiding this comment.
Desktop returns true from this. Are we explicitly deciding to return false?
There was a problem hiding this comment.
This class is not completely ported, i am just using it as a stub to get System.Timers unblocked. @AlexGhiondea is going to do the bulk porting of System.ComponentModel into a different assembly. Once his work is complete, we'll remove this.. Same for all concers in System.ComponentModel.cs, will add a comment and tracking bug.
|
|
||
| set | ||
| { | ||
| if (DesignMode) |
There was a problem hiding this comment.
If we've hardcoded DesignMode to return false and it's not virtual, do we need any of the code protected by ifs for DesignMode?
|
|
||
| int i = (int)Math.Ceiling(_interval); | ||
| _cookie = new object(); | ||
| _timer = new Threading.Timer(_callback, _cookie, i, _autoReset ? i : Timeout.Infinite); |
There was a problem hiding this comment.
Is this what the desktop implementation does? We may want to change it instead to be:
_timer = new Threading.Timer(_callback, _cookie, Timeout.Infinite, Timeout.Infinite);
_timer.Change(i, _autoReset ? i : Timeout.Infinite);Otherwise, there's a potential race condition where the timer could fire before the timer itself has been assigned back into _timer, which could cause problems if the timer callback does something like try to update the timer.
There was a problem hiding this comment.
Is this what the desktop implementation does?
Yes, Will fix in netcore.
| /// <internalonly/> | ||
| public override ISite Site | ||
| { | ||
| set |
There was a problem hiding this comment.
Nit: Can we have the getters come before the setters?
This was decided in the api-review meeting on netstandard2.0. Initially we tried to get this added to system.threading.timers contract, but the implementation of threading.timers was in mscorlib, and this in system.dll pulling in other dependencies like componentmodel. Couldn't get the lib to compile with targetingpack and other references that pulled in system.runtime. Since this type is being brought only for compat, and its dependencies on componentmodel, this was decided to be added to the componentmodel contract. |
But it's not System.ComponentModel, rather System.ComponentModel.TypeConverters. Are we renaming it to something more general? |
System.ComponentModel depends only on System.Runtime, Timers is pulling in dependencies on System.ComponentModel.Primitives, System.ComponentModel.TypeConverters, which depend on System.ComponentModel, given the other componentModel contracts, this one seemed appropriate.
I don't think so, @AlexGhiondea is looking at the ComponentModel namespace, he's investigating on introducting another contract for the rest of the members, this may get added to that.. |
|
@weshaggard, @terrajobst, what's our strategy here with these contracts? It seems like we're starting to just put things in somewhat arbitrary places. If this is the agreed upon place, ok, just seems odd.. I would never think to look ina contract named TypeConverter for a Timer. |
Yes, I had brought this up during the discussion, also why should timer be a component, but the decision was that this is only for compat, and the recommended type was to use System.Threading.Timers. |
We decided to put Timer in the "ComponentModel" bucket. Right now this is the most general ComponentModel library. @AlexGhiondea is working on the rest of ComponentModel and I think there is a decent chance it will all merged at which point this just ends up in whatever we call that merged thing. If it doesn't get merged then we have to decide where all the ComponentModel types live. |
|
@dotnet-bot test Innerloop Ubuntu14.04 Debug Build and Test |
|
@dotnet-bot test Innerloop Ubuntu14.04 Release Build and Test |
|
@dotnet-bot test Innerloop CentOS7.1 Release Build and Test |
|
@dotnet-bot test Innerloop Ubuntu14.04 Release Build and Test ('child node exited prematurely') |
| "System.ComponentModel.Primitives": "4.1.0", | ||
| "System.Globalization": "4.0.0", | ||
| "System.Reflection": "4.0.0", | ||
| "System.Runtime": "4.0.0" |
There was a problem hiding this comment.
We should probably update these package versions to use the latest prerelease instead so that we are all compatible and have the right dependencies across ns1.7
There was a problem hiding this comment.
Will address in a separate PR>
| } | ||
| "frameworks": { | ||
| "netstandard1.7": { | ||
| "dependencies": { |
There was a problem hiding this comment.
Same here, I would change this to use the prerelease packages across the board, if you don't do that then you are using older assets instead of the new ones to compile.
There was a problem hiding this comment.
Will address in a separate PR.
| <TestNugetTargetMoniker Include="$(NugetTargetMoniker)"> | ||
| <Folder>netcoreapp1.1</Folder> | ||
| </TestNugetTargetMoniker> | ||
| </ItemGroup> |
There was a problem hiding this comment.
You don't need this, this should be in dir.targets already.
There was a problem hiding this comment.
A lot of projects are still having this. is a bulk cleanup happening ?
Port System.Timers.Timer to netstandard1.7 Commit migrated from dotnet/corefx@535ba42
8 participants
Added in contract
System.ComponentModel.TypeConvertercc @danmosemsft @weshaggard @joperezr @karelz
cc @AlexGhiondea Note: the component model stubs for missing apis.
fixes #10000