Skip to content

[WIP] Fix caught callbacks#223

Closed
1j01 wants to merge 4 commits intojvilk:masterfrom
1j01:fix-caught-callbacks
Closed

[WIP] Fix caught callbacks#223
1j01 wants to merge 4 commits intojvilk:masterfrom
1j01:fix-caught-callbacks

Conversation

@1j01
Copy link
Copy Markdown
Contributor

@1j01 1j01 commented Mar 30, 2018

Fixing doubled callbacks, called once to handle results and then a second time to handle an error thrown in the callback (like described here)

Yet to do: FS.ts

Need to revisit:

  • AsyncMirror.Create
  • IndexedDBROTransaction.get
  • IndexedDBRWTransaction.put, .del
  • IndexedDBStore.clear
  • IndexedDBFileSystem.Create
  • OverlayFS.Create
  • ZipFS.Create
  • ZipFS._computeIndex

Fixes #221

1j01 added 3 commits March 30, 2018 17:38
Fix doubled callbacks, called once to handle results and then a second time to handle an error thrown in the callback

Does not compile!

Yet to do FS.ts

Need to revisit:
* AsyncMirror.Create
* IndexedDBROTransaction.get
* IndexedDBRWTransaction.put, .del
* IndexedDBStore.clear
* IndexedDBFileSystem.create
* OverlayFS.Create
* ZipFS.Create
* ZipFS._computeIndex
1j01 added a commit to 1j01/98 that referenced this pull request Mar 31, 2018
https://github.com/1j01/BrowserFS/tree/2da9e4eba77664f3fce75ea1c21cc31aaa08c846
Part of WIP pull request jvilk/BrowserFS#223

(package.json / package-lock.json not updated, but it's now linked with a version near 2.0.0-beta)
@jvilk
Copy link
Copy Markdown
Owner

jvilk commented May 1, 2018

Sorry for letting this sit for a month!

These changes look good to me. Should I merge this in, or wait for you to finish your pass through FS?

Comment thread src/backend/ZipFS.ts Outdated
err = _err;
zipTOC = _zipTOC;
};
ZipFS._computeIndexResponsive(data, index, cdPtr, cdEnd, callbackWrapper, cdEntries, eocd);
Copy link
Copy Markdown
Contributor Author

@1j01 1j01 May 1, 2018

Choose a reason for hiding this comment

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

I'm relying on _computeIndexResponsive calling back synchronously, but it's not actually always synchronous. If it was always async, I think the callback in try would be okay, but would warrant a comment like // MUST be asynchronous to avoid a double callback in _computeIndexResponsiveTrampoline

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I see; yeah, it's not always synchronous. There's a dumb hacky way to get around the issue, but I hesitate to suggest it, as it'll make the code nasty.

I can handle refactoring / fixing this specific issue, so please revert this change that you've made.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure!

@emeryberger
Copy link
Copy Markdown
Collaborator

Been a long time, lots of conflicts - please let me know if you are still interested in pursuing this or if we should close, thanks.

@emeryberger
Copy link
Copy Markdown
Collaborator

Closing for now. Can always re-open; just post a comment.

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.

Callback swallowed leading to Mutex unlock error

3 participants