Skip to content

Locking timeout#24

Open
anton-iskryzhytskyi wants to merge 5 commits into
metarhia:masterfrom
anton-iskryzhytskyi:locking-timeout
Open

Locking timeout#24
anton-iskryzhytskyi wants to merge 5 commits into
metarhia:masterfrom
anton-iskryzhytskyi:locking-timeout

Conversation

@anton-iskryzhytskyi
Copy link
Copy Markdown

Relates to #16

Comment thread web-locks.js Outdated
Comment thread web-locks.js Outdated
@tshemsedinov
Copy link
Copy Markdown
Member

This PR should not contain commits not related to timeout.

@anton-iskryzhytskyi
Copy link
Copy Markdown
Author

Updated, ready for review, @tshemsedinov

Copy link
Copy Markdown
Member

@tshemsedinov tshemsedinov left a comment

Choose a reason for hiding this comment

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

Maybe we need to use clearTimeout on abort?

Comment thread test/lock-time.js Outdated
Comment thread test/steps.js Outdated
Comment thread test/thread-worker.js Outdated
Comment thread web-locks.js Outdated
Comment thread web-locks.js Outdated
Comment thread web-locks.js Outdated
}

enter(handler) {
enter(handler, timeout) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That it much better, but we need to compare passing timeout and timer instance.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Maybe we need to use clearTimeout on abort?

Hmm, maybe it would be better to throw an error on timeout, similar to abort behavior?
Also, move all resolve / reject invocations to one place.

specify imported file extensions
@tshemsedinov
Copy link
Copy Markdown
Member

You are doing good code but mixing multiple changes into one PR, we need to talk about process.

Comment thread web-locks.js
Comment on lines +57 to +58
if (timeout)
timer = setTimeout(finalize, timeout, new TimeoutError('Time Out'));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if (timeout)
timer = setTimeout(finalize, timeout, new TimeoutError('Time Out'));
if (timeout) {
timer = setTimeout(finalize, timeout, new TimeoutError('Time Out'));
}

anton-iskryzhytskyi added a commit to anton-iskryzhytskyi/web-locks that referenced this pull request Mar 19, 2020
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