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

Fix blocking/race conditions #52

Closed
wants to merge 7 commits into from
Closed

Fix blocking/race conditions #52

wants to merge 7 commits into from

Conversation

bcicen
Copy link
Contributor

@bcicen bcicen commented Feb 20, 2018

  • Adds stop/cancel logic for internal goroutines where previously missing
  • Allow main event loop to process multiple redraw/refill/search requests on each interrupt, preventing concurrent requests from blocking each other and causing delays

This is a follow-up to (and depends on) #51.

@bcicen bcicen mentioned this pull request Feb 20, 2018
@tigrawap
Copy link
Owner

Looks fantastic

I want to play with it just a bit before merging. I assume merging this one and closing initial one will do

A question, how NewFromStream is used? I assume you already integrated in some app of yours? Or just a common-sense method?

@tigrawap
Copy link
Owner

Wanted to check how far it's from making search non blocking (at least cancellable), on top of this branch.

Found few issues so far:

  • Line scan takes longer now. Checking on 65MB file (342280 lines)
    On master version it's pretty immediate, on this branch takes few secs
  • Long empty search (i.e start, search for alsdfjlksadjfl) + press q in the middle - on master q stays in termbox events loop and term exits once search is done, on this branch - it's kinda discarded and does nothing later on (since event loop is already break-en)

@tigrawap
Copy link
Owner

tigrawap commented Feb 25, 2018

Looks like problem with interrupt termbox.Interrupt() within updateLastLine
Changing it back to following fixes both issues
it's always should be interrupt followed by call to actual action(which will block until processed)
Otherwise every next loop of line scan was blocked by 10ms select-sleep within new event loop(since termbox.Interrupt() was blocked until it is picked up)

				go termbox.Interrupt()
				select {
				case requestStatusUpdate <- lastLine.Line:
					f.fetcher.updateMap(dataLine)
				case <-ctx.Done():
					return
				}

@tigrawap
Copy link
Owner

tigrawap commented Feb 25, 2018

pushed followups to bcicen-fix-blocking-conditions, it sounds like more general approach for handling events should be taken, i.e wrap termbox.PollEvent() (and?) interrupt, instead of processing multiple requests inside single interrupt event.

(i.e pre-fetch and process from own queue, with ability to flush all blocked on-interrupt go-routines or stop buffer.fill() context if some control sequences(ctrl+c?) received while processing previous event)
I'd say it big enough to be outside of scope of this PR.

What was the problem you was fixing with this change? (i've basically reverted it in followups...). Maybe it can be solved separately, but differently

@tigrawap
Copy link
Owner

Opened #53 to follow up on blocking issue with general idea for solving. I hope to find a time to work on it this week, if you will want to take over please LMK

See follow up branch(bcicen-fix-blocking-conditions), repush it here if it's fine with you(in case i've missed something why this change was essential)

@tigrawap
Copy link
Owner

Merged manually, with comments above

@tigrawap tigrawap closed this Apr 22, 2018
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.

2 participants