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

Use per-instance Mono_Time instead of a global unix_time #1038

Merged
merged 1 commit into from
Aug 17, 2018

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Aug 1, 2018

This avoids some global state.


This change is Reviewable

@iphydf iphydf added this to the v0.2.x milestone Aug 1, 2018
@iphydf iphydf force-pushed the local-mono-time branch 2 times, most recently from 62a6253 to 328f3ed Compare August 2, 2018 00:06
@iphydf iphydf changed the title Use per-instance Mono_Time for Messenger and onion. WIP: Use per-instance Mono_Time for Messenger and onion. Aug 2, 2018
@iphydf iphydf force-pushed the local-mono-time branch 4 times, most recently from f528dc0 to 93fe30b Compare August 4, 2018 12:13
@iphydf iphydf changed the title WIP: Use per-instance Mono_Time for Messenger and onion. Use per-instance Mono_Time for Messenger and onion. Aug 4, 2018
@iphydf iphydf unassigned zugz Aug 4, 2018
zugz
zugz previously approved these changes Aug 8, 2018
@nurupo
Copy link
Member

nurupo commented Aug 8, 2018

I'm so confused, there is no explanation either in PR or the commit message as to why this is needed, what improvement this brings. At first I thought that you moved the Windows'es global state into a per-instance struct, but that's not the case.

Also, what is the point of the base_time?

@nurupo
Copy link
Member

nurupo commented Aug 8, 2018

Oh, nevermind, diff of mono_time.c explains it all. You just removed the the global state struct, nothing else changed, the base_time was there alredy.

@zugz zugz dismissed their stale review August 9, 2018 03:07

second thoughts

@zugz
Copy link

zugz commented Aug 9, 2018

I'm having second thoughts on this. Could you explain why this is a positive change?

Mono_Time is meant to represent actual unix time. This is a global concept, so doesn't it actually make sense to use a global for it? Meanwhile making it per-instance increases code complexity.

Having it as a global also makes time manipulation in tests easier.

The only practical advantage I see of making it per-instance is to allow us to model relative clock differences between instances in tests, which I don't anticipate us actually wanting to do.

@iphydf iphydf force-pushed the local-mono-time branch 4 times, most recently from 14b5321 to a0dec00 Compare August 10, 2018 15:49
@iphydf iphydf changed the title Use per-instance Mono_Time for Messenger and onion. Use per-instance Mono_Time instead of a global unix_time Aug 10, 2018
@iphydf iphydf force-pushed the local-mono-time branch 2 times, most recently from 81b1667 to cd91b57 Compare August 11, 2018 16:39
@iphydf
Copy link
Member Author

iphydf commented Aug 12, 2018

@zugz PTAL. I've added a comment to explain the assumptions we want to make in the event loop.

Copy link

@zugz zugz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'm happy now.

@zugz
Copy link

zugz commented Aug 12, 2018

:lgtm_strong:

@iphydf iphydf force-pushed the local-mono-time branch 2 times, most recently from 690fb51 to 253b004 Compare August 14, 2018 22:05
@iphydf iphydf merged commit d6d305f into TokTok:master Aug 17, 2018
@iphydf iphydf deleted the local-mono-time branch August 17, 2018 22:33
@robinlinden robinlinden modified the milestones: v0.2.x, v0.2.7 Aug 30, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants