Skip to content
This repository has been archived by the owner on Nov 21, 2020. It is now read-only.

unhandled rejection from local-fs #41

Closed
tyv opened this issue Mar 1, 2017 · 16 comments
Closed

unhandled rejection from local-fs #41

tyv opened this issue Mar 1, 2017 · 16 comments
Assignees
Labels

Comments

@tyv
Copy link
Contributor

tyv commented Mar 1, 2017

we have unhandled rejection here in loadTextFile: https://github.com/wix/kissfs/blob/master/src/local-fs.ts#L69-L83

it happens if watcher for some reason have event from ignored folder.

@amir-arad
Copy link
Contributor

we get an event on a folder we configured chokidar to ignore? so this is a bug in chokidar?

@amir-arad amir-arad added the bug label Mar 3, 2017
@tyv
Copy link
Contributor Author

tyv commented Mar 3, 2017

Probably we have to rename issue.

We had bug in ignoring, thats why I moved to micromatch and yet we had issue with that.
Chokidar is giving you or anymatch compatible api, or function that returns boolean for given path.
We use now function to give simpler configuration api then chokidar.

So now we don't have unexpected event (cause I've fixed bugs I mentioned above), but if for some reason (it could be a lot of them) loadTextFile will be rejected after correct chokidar event — we can't know that.

I'd prefer to have a special kissfs api event for that, but we have discussion in PR #42 about that.. so we have probability of errors there which we don't have API to handle. yet.

@amir-arad
Copy link
Contributor

a concrete scenario in the form of code would help me understand better

@tyv
Copy link
Contributor Author

tyv commented Mar 3, 2017

umm. scenario of possibility?
for example chokidar reported 'add' and then:

  • OS filesystem failed => loadTextFile() rejected
  • smth changed permissions => loadTextFile() rejected
  • HDD/SSD broke => loadTextFile() rejected
  • Jusus don't love you anymore => loadTextFile() rejected
    ...

@amir-arad
Copy link
Contributor

I think I follow.
I guess we want to soften the blow by adding retries, but eventually it boils down to either that file not existing (created/changed and immediately later deleted), in which case we will get a deletion event at some point, or, the entire filesystem failing, at which case there is really not much to do in the scope of a single file. (but we do need to signal it).

@tyv
Copy link
Contributor Author

tyv commented Mar 6, 2017

@amir-arad so.. if we're

  • considering third party to be involved in fs process
  • that processes are async
  • and there might be collisions

I suggest to add that event to API back.
also for cache-fs we can add constructor flag, says to recache tree on errors

@amir-arad
Copy link
Contributor

👍

@amir-arad
Copy link
Contributor

amir-arad commented Mar 6, 2017

so we have:

  1. test + add retries for above scenario when a 3rd party changes underlying filesystem
  2. test + add generic "unexpected error" event with no parameter to the API
  3. test + add 'reset on error' behavior for cache that does not pass the error event forward but instead re-scans the tree and updates cache and emits events as required. That's a big one.

@tyv
Copy link
Contributor Author

tyv commented Mar 6, 2017

and emits events as required. That's a big one.

thats because of diff?

@amir-arad
Copy link
Contributor

yes

@tyv
Copy link
Contributor Author

tyv commented Mar 14, 2017

test + add retries for above scenario when a 3rd party changes underlying filesystem

think this is possible only with placing watcher as constructor option with chokidar default.
gonna do that together with #46

@amir-arad
Copy link
Contributor

What's the problem with with this scenario using chokidar the same way as it is done today?

@tyv
Copy link
Contributor Author

tyv commented Mar 15, 2017

@amir-arad I need test suite when we will trigger events from watcher

@amir-arad
Copy link
Contributor

@tyv that will not really simulate a change to the file system, only a change event (or the absence of change events).

@tyv
Copy link
Contributor Author

tyv commented Mar 15, 2017

@amir-arad how you can see that I will emulate this?

I think I follow.
I guess we want to soften the blow by adding retries, but eventually it boils down to either that file not existing (created/changed and immediately later deleted), in which case we will get a deletion event at some point, or, the entire filesystem failing, at which case there is really not much to do in the scope of a single file. (but we do need to signal it).

@tyv
Copy link
Contributor Author

tyv commented Mar 15, 2017

as we need an unknown error

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants