Skip to content

Commit

Permalink
Add timeouts so missing/idle streams can't prevent LabRecorder from s…
Browse files Browse the repository at this point in the history 
…hutting down. Thanks to Jason Kowaleski (@kowalej)
  • Loading branch information
@tstenner
tstenner committed Dec 14, 2018
1 parent dd199bc commit fded925
Showing 1 changed file with 4 additions and 2 deletions.
6 changes: 4 additions & 2 deletions recording.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -373,14 +373,16 @@ void recording::typed_transfer_loop(streamid_t streamid, double srate, const inl
std::vector<double> timestamps;

// Pull the first sample
first_timestamp = last_timestamp = in->pull_sample(chunk);
first_timestamp = 0.0;
while(first_timestamp == 0.0)
first_timestamp = last_timestamp = in->pull_sample(chunk, 4.0);
timestamps.push_back(first_timestamp);
file_.write_data_chunk(streamid, timestamps, chunk, in->get_channel_count());

auto next_pull = Clock::now();
while (!shutdown_) {
// get a chunk from the stream
in->pull_chunk_multiplexed(chunk, &timestamps);
in->pull_chunk_multiplexed(chunk, &timestamps, 4.0);
// for each sample...
for (double &ts : timestamps) {
// if the time stamp can be deduced from the previous one...
Expand Down

10 comments on commit fded925

@xbbsky
Copy link

@xbbsky xbbsky commented on fded925 Jan 10, 2019

Choose a reason for hiding this comment

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

in line 385, in->pull_chunk_multiplexed(chunk, &timestamps, 4.0), the parameter timeout=4.0 makes this line kind of a dead loop. This prevents the write data process and makes the chunk too big. The timeout here should be 0.0.

@tstenner
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xbbsky It's needed because we need to check if the shutdown flag was set periodically and without a timeout this will only happen once a chunk was received and hang otherwise

@xbbsky
Copy link

@xbbsky xbbsky commented on fded925 Jan 10, 2019

Choose a reason for hiding this comment

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

So here the timeout should be a very small value, such as 1e-6. 4.0 is too big.

@cboulay
Copy link
Contributor

Choose a reason for hiding this comment

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

This fixes a lot of problems for me.
@tstenner How can I test for the scenario that the timeout was trying to address? Do I make a stream that isn't closed but isn't pushing anything?

@tstenner
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cboulay I think I committed an example app that creates an outlet without sending data, but I don't know if pushed it to the examples repo

@tstenner
Copy link
Contributor Author

Choose a reason for hiding this comment

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

4.0 is probably too big, but it shouldn't be less than a second.

@cboulay
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure that units are seconds? I didn't wait a full 4 minutes, but I wanted longer than 30 seconds and the app remained frozen until the remote streams were closed and LabRecorder caught an exception.

@tstenner
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All LSL doubles are in seconds, so unless pull_chunk does something wrong it should be a maximum of 4 seconds.

@cboulay
Copy link
Contributor

Choose a reason for hiding this comment

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

Then it's doing something wrong because it's much longer than 4 seconds.

@tstenner
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I'll take care of it.

Please sign in to comment.