Skip to content

Commit

Permalink
Second attempt at tailing file.
Browse files Browse the repository at this point in the history
  • Loading branch information
sjoshid committed Nov 10, 2019
1 parent 0b15cfe commit 4f437a3
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 14 deletions.
32 changes: 31 additions & 1 deletion rust/core-lib/src/file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use std::collections::HashMap;
use std::ffi::OsString;
use std::fmt;
use std::fs::{self, File};
use std::io::{self, Read, Write};
use std::io::{self, Read, Seek, SeekFrom, Write};
use std::path::{Path, PathBuf};
use std::str;
use std::time::SystemTime;
Expand Down Expand Up @@ -54,6 +54,8 @@ pub struct FileInfo {
pub has_changed: bool,
#[cfg(target_family = "unix")]
pub permissions: Option<u32>,
#[cfg(feature = "notify")]
pub tailing_enabled: Option<u64>,

This comment has been minimized.

Copy link
@sjoshid

sjoshid Nov 10, 2019

Author Owner

Some(u64) will contain cursor location in file which I'll keep updating as I read delta.
If None, means tailing for that file is disabled.

}

pub enum FileError {
Expand Down Expand Up @@ -147,6 +149,8 @@ impl FileManager {
has_changed: false,
#[cfg(target_family = "unix")]
permissions: get_permissions(path),
#[cfg(feature = "notify")]
tailing_enabled: None,
};
self.open_files.insert(path.to_owned(), id);
self.file_info.insert(id, info);
Expand All @@ -172,6 +176,29 @@ impl FileManager {
}
Ok(())
}

#[cfg(feature = "notify")]
pub fn update_current_position_in_tail(
&mut self,
path: &Path,
id: BufferId,
) -> Result<(), FileError> {
let existing_file_info = self.file_info.get_mut(&id).unwrap();

let mut f = File::open(path).map_err(|e| FileError::Io(e, path.to_owned()))?;
let end_position =
f.seek(SeekFrom::End(0)).map_err(|e| FileError::Io(e, path.to_owned()))?;

existing_file_info.tailing_enabled = Some(end_position);
Ok(())
}

#[cfg(feature = "notify")]
pub fn disable_tailing(&mut self, id: BufferId) -> Result<(), FileError> {
let existing_file_info = self.file_info.get_mut(&id).unwrap();
existing_file_info.tailing_enabled = None;
Ok(())
}
}

fn try_load_file<P>(path: P) -> Result<(Rope, FileInfo), FileError>
Expand All @@ -194,6 +221,9 @@ where
permissions: get_permissions(&path),
path: path.as_ref().to_owned(),
has_changed: false,
//sj_todo tailing_enabled should be set from existing FileInfo.
#[cfg(feature = "notify")]
tailing_enabled: None,
};
Ok((rope, info))
}
Expand Down
2 changes: 2 additions & 0 deletions rust/core-lib/src/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,8 @@ pub enum CoreNotification {
SaveTrace { destination: PathBuf, frontend_samples: Value },
/// Tells `xi-core` to set the language id for the view.
SetLanguage { view_id: ViewId, language_id: LanguageId },
/// Enables/disabled tailing file at file_path.
ToggleTail { view_id: ViewId, file_path: String, enabled: bool },

This comment has been minimized.

Copy link
@cmyr

cmyr Nov 10, 2019

do we need to pass the path here? if this is 'toggling', i would expect the view to already have a file open, or not

This comment has been minimized.

Copy link
@sjoshid

sjoshid Nov 17, 2019

Author Owner

You are right. I have made the change in the commit.

}

/// The requests which make up the base of the protocol.
Expand Down
83 changes: 72 additions & 11 deletions rust/core-lib/src/tabs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ use crate::WeakXiCore;

#[cfg(feature = "notify")]
use crate::watcher::{FileWatcher, WatchToken};
use notify::event::Flag::Ongoing;
#[cfg(feature = "notify")]
use notify::Event;
#[cfg(feature = "notify")]
Expand Down Expand Up @@ -331,6 +332,9 @@ impl CoreState {
// handled at the top level
ClientStarted { .. } => (),
SetLanguage { view_id, language_id } => self.do_set_language(view_id, language_id),
ToggleTail { view_id, file_path, enabled } => {
self.do_toggle_tail(view_id, file_path, enabled)
}
}
}

Expand Down Expand Up @@ -555,6 +559,32 @@ impl CoreState {
fn after_stop_plugin(&mut self, plugin: &Plugin) {
self.iter_groups().for_each(|mut cx| cx.plugin_stopped(plugin));
}

#[cfg(feature = "notify")]
fn do_toggle_tail(&mut self, view_id: ViewId, path: P, enabled: bool)
where
P: AsRef<Path>,
{
let buffer_id = self.views.get(&view_id).map(|v| v.borrow().get_buffer_id());
let buffer_id = match buffer_id {
Some(id) => id,
None => return,
};
if enabled {
let path = path.as_ref();
self.file_manager.update_current_position_in_tail(path, buffer_id);
} else {
self.file_manager.disable_tailing(buffer_id);
}
}

#[cfg(not(feature = "notify"))]
fn do_toggle_tail(&mut self, view_id: ViewId, path: P, enabled: bool)
where
P: AsRef<Path>,
{
warn!("do_toggle_tail called without notify feature enabled.");
}
}

/// Idle, tracing, and file event handling
Expand Down Expand Up @@ -705,10 +735,36 @@ impl CoreState {
#[cfg(feature = "notify")]
fn handle_open_file_fs_event(&mut self, event: Event) {
use notify::event::*;
let mut is_tail_event = false;
let path = match event.kind {
EventKind::Create(CreateKind::Any)
| EventKind::Modify(ModifyKind::Metadata(MetadataKind::Any))
| EventKind::Modify(ModifyKind::Any) => &event.paths[0],
| EventKind::Modify(ModifyKind::Any) => {
let path = &event.paths[0];

is_tail_event = match event.flag().unwrap_or_else(false) {

This comment has been minimized.

Copy link
@sjoshid

sjoshid Nov 10, 2019

Author Owner

This is ugly :(

This comment has been minimized.

Copy link
@cmyr

cmyr Nov 10, 2019

indeed. it isn't totally clear what you're trying to do here, but I bet this could be cleaned up.
event.flag() returns an Option?

Is this maybe the same logic?

if event.flag().unwrap_or(false) {
    let buf_id = self.file_manager.get_editor(path);
    let trailing = buf_id.and_then(|id| {
        self.file_manager
        .get_info(id)
        .map(|info| info.trailing_enabled)
    });
    is_tail_event = trailing.unwrap_or(false);
}

This comment has been minimized.

Copy link
@sjoshid

sjoshid Nov 10, 2019

Author Owner

event.flag() returns Option<&Flag> where Flag is enum. Ongoing is a variant. So idea is

  1. Check if the flag is Ongoing. If None or some other Flag type, we continue. Maybe this is regular event.
  2. If Ongoing, we check if tail is enabled for incoming path (this information is stored in FileInfo). If tail disabled for path, we IGNORE this event. Saving us from expensive reload. If tail enabled for path, we set is_tail_event to true and reload only delta.
Ongoing => {
let buffer_id = match self.file_manager.get_editor(path) {
Some(id) => id,
None => return,
};
let file_info = self.file_manager.get_info(buffer_id);
match file_info {
Some(i) => {
match i.tailing_enabled {
Some(t) => true,
// Ignore tail events for paths which are not tailed.
None => return,
}
}
None => return,
}
}
_ => false,
};

path
}
other => {
debug!("Ignoring event in open file {:?}", other);
return;
Expand All @@ -727,16 +783,21 @@ impl CoreState {
// A more robust solution would also hash the file's contents.

if has_changes && is_pristine {
if let Ok(text) = self.file_manager.open(path, buffer_id) {
// this is ugly; we don't map buffer_id -> view_id anywhere
// but we know we must have a view.
let view_id = self
.views
.values()
.find(|v| v.borrow().get_buffer_id() == buffer_id)
.map(|v| v.borrow().get_view_id())
.unwrap();
self.make_context(view_id).unwrap().reload(text);
if is_tail_event {

This comment has been minimized.

Copy link
@sjoshid

sjoshid Nov 10, 2019

Author Owner

Impl is remaining but here instead of loading the whole file, Im going to load delta only.

// Since this is tail event, we need to get delta of changes since the last time we
// read the file.
} else {
if let Ok(text) = self.file_manager.open(path, buffer_id) {
// this is ugly; we don't map buffer_id -> view_id anywhere
// but we know we must have a view.
let view_id = self
.views
.values()
.find(|v| v.borrow().get_buffer_id() == buffer_id)
.map(|v| v.borrow().get_view_id())
.unwrap();
self.make_context(view_id).unwrap().reload(text);
}
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions rust/core-lib/src/watcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
//! they arrive, and an idle task is scheduled.

use crossbeam::unbounded;
use notify::{event::*, watcher, RecommendedWatcher, RecursiveMode, Watcher};
use notify::{event::*, watcher, Config, RecommendedWatcher, RecursiveMode, Watcher};
use std::collections::VecDeque;
use std::fmt;
use std::mem;
Expand All @@ -44,7 +44,9 @@ use std::sync::{Arc, Mutex};
use std::thread;
use std::time::Duration;

use notify::event::DataChange::Content;
use xi_rpc::RpcPeer;
use xi_trace::SampleEventType::DurationBegin;

/// Delay for aggregating related file system events.
pub const DEBOUNCE_WAIT_MILLIS: u64 = 50;
Expand Down Expand Up @@ -98,7 +100,9 @@ impl FileWatcher {
let state = Arc::new(Mutex::new(WatcherState::default()));
let state_clone = state.clone();

let inner = watcher(tx_event, Duration::from_millis(100)).expect("watcher should spawn");
let mut inner =
watcher(tx_event, Duration::from_millis(100)).expect("watcher should spawn");
inner.configure(Config::OngoingEvents(Some(Duration::from_millis(50))));

This comment has been minimized.

Copy link
@sjoshid

sjoshid Nov 10, 2019

Author Owner

I think I missed this important change. In newer Notify, we can CONFIGURE watcher to send us OngoingEvents which are emitted before debounced time. Im handling these Ongoing events to find delta.

In an ideal world, we can turn on/off these events for every open file (because tailing is per file) but not in this case. Newer notify makes it possible to add new watcher for same source (thanks to crossbeam channels which are not used in older notify) but that would mean have starting/shutting down new watcher for every file Im tailing. I dont think this is correct.
See here.

By making this change OngoingEvents will be emitted as default. So notify will emit Ongoing event even if none of the files are being tailed. This does not mean Im actually using Ongoing event. handle_open_file_fs_event will filter out such event if tailing is off for that path (Refer ugly impl) saving us from more expensive reload.


thread::spawn(move || {
while let Ok(Ok(event)) = rx_event.recv() {
Expand Down

3 comments on commit 4f437a3

@sjoshid
Copy link
Owner Author

Choose a reason for hiding this comment

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

@cmyr
Would it be possible to a quick review of this? I'll open a PR if you are ok with the general direction.

@cmyr
Copy link

@cmyr cmyr commented on 4f437a3 Nov 10, 2019

Choose a reason for hiding this comment

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

nothing here seems crazy!

@sjoshid
Copy link
Owner Author

Choose a reason for hiding this comment

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

Added few more comments.

Please sign in to comment.