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.

Sign up for GitHub

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

Jump to bottom

Tokio 0.2.7 #161

Merged
merged 3 commits into from
Jan 8, 2020
Merged

Tokio 0.2.7 #161

merged 3 commits into from
Jan 8, 2020

Conversation

osa1
Copy link
Owner

@osa1 osa1 commented Dec 5, 2019

Fixes #160

@osa1
Copy link
Owner Author

osa1 commented Dec 6, 2019

Almost there.. Shutdown doesn't work, closing the TUI leaves clients hanging, I think.

@osa1
Copy link
Owner Author

osa1 commented Dec 6, 2019

Indeed, C-c makes the process hang in a busy loop.

@osa1
Copy link
Owner Author

osa1 commented Dec 6, 2019

Backtrace:

>>> bt
#0  0x00005565313e4218 in <tokio::task::local::Scheduler as core::ops::drop::Drop>::drop (self=0x556532e724b0)
at /home/omer/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-0.2.2/src/task/local.rs:499
#1  0x0000556530f4f435 in core::ptr::real_drop_in_place () at /rustc/412f43ac5b4ae8c3599e71c6972112e9be4758fa/src/libcore/ptr/mod.rs:181
#2  0x0000556530f52cc7 in core::ptr::real_drop_in_place () at /rustc/412f43ac5b4ae8c3599e71c6972112e9be4758fa/src/libcore/ptr/mod.rs:181
#3  0x0000556530f4ac34 in core::ptr::drop_in_place (to_drop=0x556532e724a0) at /rustc/412f43ac5b4ae8c3599e71c6972112e9be4758fa/src/libcore/ptr/mod.rs:171
#4  <alloc::rc::Rc<T> as core::ops::drop::Drop>::drop (self=0x7ffddcf2ff00) at /rustc/412f43ac5b4ae8c3599e71c6972112e9be4758fa/src/liballoc/rc.rs:1101
#5  0x0000556530f52f9e in core::ptr::real_drop_in_place () at /rustc/412f43ac5b4ae8c3599e71c6972112e9be4758fa/src/libcore/ptr/mod.rs:181
#6  0x0000556530f53cce in core::ptr::real_drop_in_place () at /rustc/412f43ac5b4ae8c3599e71c6972112e9be4758fa/src/libcore/ptr/mod.rs:181
#7  0x0000556530f0b7ea in tiny::run (servers=..., defaults=..., colors=..., config_path=..., log_dir=...) at tiny/src/main.rs:171
#8  0x0000556530f0b1ce in tiny::main () at tiny/src/main.rs:64

@osa1
Copy link
Owner Author

osa1 commented Dec 6, 2019

I think it's this loop in Drop impl for Scheduler:

            // Wait until all tasks have been released.
            // XXX: this is a busy loop, but we don't really have any way to park
            // the thread here?
            while self.queues.has_tasks_remaining() {
                self.queues.drain_pending_drop();
            }

In any case the shutdown code is not quite right anyway -- LocalSet::block_on does not wait for all tasks to finish which is not right. The code already structured to gracefully close all connections on Quit command so we should be waiting for all tasks to finish instead of killing them by returning from the main task in LocalSet::block_on.

@osa1
Copy link
Owner Author

osa1 commented Dec 6, 2019

Perf output of the loop:

Screenshot_2019-12-06_09-44-56

@osa1
Copy link
Owner Author

osa1 commented Dec 6, 2019

The loop is fixed on tokio master. We still need a way to wait on all tasks of a LocalSet though. Tokio tracking issue: tokio-rs/tokio#1908

@osa1 osa1 changed the title Tokio 0.2.2 Tokio 0.2.3 Dec 6, 2019
@osa1
Copy link
Owner Author

osa1 commented Dec 6, 2019

Tokio 0.2.3 is just released, and it fixes the hanging on shutdown. We still don't gracefully terminate the tasks.

@osa1 osa1 changed the title Tokio 0.2.3 Tokio 0.2.4 Dec 7, 2019
@osa1 osa1 mentioned this pull request Dec 24, 2019
Merged
osa1 added a commit that referenced this pull request Dec 26, 2019
@osa1
Implement user configurable timestamp formatting
774117b 
New optional config file field `timestamp_format` used as `strftime`
format string used to show timestamps.

To support `/reload` command `Line` now has a new `Option<Tm>` field.
When format string is changed we visit all lines and update timestamp
strings.

(If this turns out to be too slow in practice we can re-generate
timestamp strings lazily)

Rendering performance should be the same: we still hold formatted
strings in message areas, so instead of e.g. doing strftime every time a
line is rendered we do the formatting once.

Fixes #161
osa1 added a commit that referenced this pull request Dec 26, 2019
@osa1
Implement user configurable timestamp formatting
48d3a73 
New optional config file field `timestamp_format` used as `strftime`
format string used to show timestamps.

To support `/reload` command `Line` now has a new `Option<Tm>` field.
When format string is changed we visit all lines and update timestamp
strings.

(If this turns out to be too slow in practice we can re-generate
timestamp strings lazily)

Rendering performance should be the same: we still hold formatted
strings in message areas, so instead of e.g. doing strftime every time a
line is rendered we do the formatting once.

Fixes #161
@osa1 osa1 changed the title Tokio 0.2.4 Tokio 0.2.6 Dec 29, 2019
@osa1 osa1 force-pushed the tokio-0.2.2 branch 2 times, most recently from 9cabe91 to 4a21e63 Compare January 4, 2020 15:21
@osa1
Switch to tokio-0.2.7 and (non-preview) futures
12bc0f8 
Fixes #160
@osa1
Simplify UI task termination logic, fix a race between clients and UI
dfce148 
Quitting tiny (C-c in TUI) does not mean all clients will immediately
stop: they'll send a QUIT message to the server, and depending on
scheduling they may even receive a message or two. In those cases if we
return from TUI right away when quitting we'd get panics in client code
because client assumes the receiving end of the connection event channel
is always open until the client returns.

We now keep the UI task running until all client channels are dropped.
Because the UI task is the owner of the TUI this avoids the race (TUI is
awailable as long as there's at least one client running) by keeping the
TUI alive.

A good side effect of this is that after C-c we won't be waiting in the
shell for the process to exit (i.e. clients to send QUIT), instead we'll
see the TUI.
@osa1 osa1 changed the title Tokio 0.2.6 Tokio 0.2.7 Jan 8, 2020
@osa1
Copy link
Owner Author

osa1 commented Jan 8, 2020

This is good to go.

@osa1 osa1 mentioned this pull request Jan 8, 2020
Closed
@osa1 osa1 merged commit fda177e into master Jan 8, 2020
@osa1 osa1 deleted the tokio-0.2.2 branch October 12, 2020 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

Switch over to tokio-0.2 (and drop preview versions of futures?)
1 participant
@osa1