Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

fix: do not tick after stop #13

Merged
merged 6 commits into from
Dec 27, 2021
Merged

fix: do not tick after stop #13

merged 6 commits into from
Dec 27, 2021

Conversation

tie
Copy link
Contributor

@tie tie commented Dec 27, 2021

This PR

  • Adds a test for a quirk in the implementation: observe channel gets closed on each ticker’s tick (since it plans a new moment).
  • Fixes Ticker’s Stop method to remove a new moment created by tick that would previously cause a ticker to run indefinitely (as long as the time is going forward).
  • Fixes Time’s Set method to run temporal effects under the lock.

@ernado, not sure if the first point was an intentional behavior. I don’t think there are other public projects that use neo (see pkg.go.dev), so perhaps we can fix it in a separate PR if td tests do not rely on this quirk?

@tie
fix: update ticker moment ID on tick
12329c1 
Stopping a ticker after the first tick before this change was a no-op
since the Ticker’s moment ID was not updated.
@tie
Copy link
Contributor Author

tie commented Dec 27, 2021 •
edited
Loading

Uh, net.go tests seem to be flaky. I’ll force-push to rerun the tests.

panic: send on closed channel

goroutine 19 [running]:
github.com/gotd/neo.(*PacketConn).WriteTo(0xc000098050, 0xc000114c00, 0x5, 0x400, 0x65fac0, 0xc0000a4060, 0xc0000a4060, 0x0, 0x0)
        /home/runner/work/neo/neo/net.go:88 +0x37b
github.com/gotd/neo.(*pingServer).Listen(0xc00009e010, 0x0, 0x0)
        /home/runner/work/neo/neo/net_test.go:51 +0x142
github.com/gotd/neo.TestNetPingDeadline.func1(0xc00009e010, 0xc000082300, 0xc0000a20c0)
        /home/runner/work/neo/neo/net_test.go:143 +0x3d
created by github.com/gotd/neo.TestNetPingDeadline
        /home/runner/work/neo/neo/net_test.go:142 +0x325
FAIL    github.com/gotd/neo     0.115s  
?       github.com/gotd/neo/examples    [no test files]
FAIL

=== RUN   TestNetPingDeadline
==================
WARNING: DATA RACE
Read at 0x00c000122070 by goroutine 11:
  runtime.chansend()
      runtime/chan.go:159 +0x0
  github.com/gotd/neo.(*PacketConn).WriteTo()
      github.com/gotd/neo/net.go:89 +0x2a4
  github.com/gotd/neo.(*pingServer).Listen()
      github.com/gotd/neo/net_test.go:51 +0xc1
  github.com/gotd/neo.TestNetPingDeadline.func1()
      github.com/gotd/neo/net_test.go:143 +0x46

Previous write at 0x00c000122070 by goroutine 10:
  runtime.closechan()
      runtime/chan.go:356 +0x0
  github.com/gotd/neo.(*PacketConn).Close()
      github.com/gotd/neo/net.go:114 +0x134
  github.com/gotd/neo.TestNetPingDeadline()
      github.com/gotd/neo/net_test.go:165 +0x4fa
  testing.tRunner()
      testing/testing.go:1410 +0x213
  testing.(*T).Run.func1()
      testing/testing.go:1457 +0x47

Goroutine 11 (running) created at:
  github.com/gotd/neo.TestNetPingDeadline()
      github.com/gotd/neo/net_test.go:142 +0x38b
  testing.tRunner()
      testing/testing.go:1410 +0x213
  testing.(*T).Run.func1()
      testing/testing.go:1457 +0x47

Goroutine 10 (running) created at:
  testing.(*T).Run()
      testing/testing.go:1457 +0x724
  testing.runTests.func1()
      testing/testing.go:1810 +0x99
  testing.tRunner()
      testing/testing.go:1410 +0x213
  testing.runTests()
      testing/testing.go:1808 +0x7e4
  testing.(*M).Run()
      testing/testing.go:1690 +0xa71
  main.main()
      _testmain.go:67 +0x2e4
==================
panic: send on closed channel

goroutine 19 [running]:
github.com/gotd/neo.(*PacketConn).WriteTo(0xc000124000, {0xc00012a000, 0x5, 0x0?}, {0x5fe3d8, 0xc00011e060})
        github.com/gotd/neo/net.go:88 +0x2fa
github.com/gotd/neo.(*pingServer).Listen(0xc000112030)
        github.com/gotd/neo/net_test.go:51 +0xc2
github.com/gotd/neo.TestNetPingDeadline.func1()
        github.com/gotd/neo/net_test.go:143 +0x47
created by github.com/gotd/neo.TestNetPingDeadline
        github.com/gotd/neo/net_test.go:142 +0x38c
exit status 2
FAIL    github.com/gotd/neo     0.191s

@tie
refactor: implement do method for timer
797add8 
This change adds a do method for timer (similar to ticker’s do method).
It also adds a test case to verifies that backwards compatibility is
preserved for a quirk in the current Observe and ticker implementation.

That is, a tick triggers the observeUnlocked method. I’m not aware of
any piece of code actually relying on this behavior, but it’s better to
change that separately if we want to.

Additionally, we introduce a small convention for Time’s methods: if the
method does not acquire the lock, it has Unlocked suffix. A notable
exception is the do method on timer and ticker that is expected to run
under Time’s lock when scheduled temporal effects are triggered.
@tie tie mentioned this pull request Dec 27, 2021
Closed
@coveralls
Copy link

coveralls commented Dec 27, 2021 •
edited
Loading

Pull Request Test Coverage Report for Build 1625591901

  • 55 of 55 (100.0%) changed or added relevant lines in 3 files are covered.
  • 16 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-4.7%) to 76.792%
Files with Coverage Reduction New Missed Lines %
net.go 16 49.59%
Totals Coverage Status
Change from base Build 1515030801: -4.7%
Covered Lines: 225
Relevant Lines: 293
💛 - Coveralls

@tie
Copy link
Contributor Author

tie commented Dec 27, 2021 •
edited
Loading

I’ve added t.Skip for now and created a new issue.

@tie tie force-pushed the fix-ticker-stop branch 2 times, most recently from 49df95d to 8e7d13d Compare December 27, 2021 02:33
@tie
fix: do not create a new moment for each tick
ac80857 
This change avoid creating a new moment for each Ticker’s tick but still
maintains backwards compatibility for tick observeration. It also fixes
an accidentally introduced data race for id field between Reset and do
methods in ticker.
@ernado ernado merged commit c20eb63 into gotd:main Dec 27, 2021
@tie tie deleted the fix-ticker-stop branch December 27, 2021 09:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tie @coveralls @ernado