fix: handle outdated message in channel queue

[jedlikowski]

Fixes #183

[changeset-bot]

🦋 Changeset detected

Latest commit: 44881df

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
synckit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[JounQin]

What's the issue here? Can you provide more information? Reproduction or failing test for example.

[jedlikowski]

Hi, sorry, it was meant to be a draft PR ;) I've linked in the description to the issue this PR means to fix.

[jedlikowski]

Hi, I've added one more change to avoid sending the worker task result to parent if parent isn't waiting for the result anymore. Let me know what you think of this PR and I'll be happy to carry it to the end :)

I also wondered how we could pass some sort of EventEmitter to the task function itself, to let it know that the task has been aborted. This would allow it for example to cancel a HTTP request, but I couldn't figure out a way to do it without changing the library API.

This chart aims visualize the current behavior (dotted line means Atomics.wait() timeout on parent process):

While this one shows the updated behavior:

[jedlikowski]

Hi, @JounQin, what do you think of this PR? Would you be interested in such a contribution?

[KubaZ]

Hi @JounQin, could you please take a look at this PR?

[JounQin]

Interesting, is that possible to add a related test case?

[jedlikowski]

Interesting, is that possible to add a related test case?

Hi @JounQin , I've added a test case that will currently fail on main branch and pass on this branch: https://github.com/un-ts/synckit/pull/184/files#diff-708937a178903f6cba26c362f7fda7d1da8fb676b66c79dbd63fbffdc7a2db96R107-R125

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (35a89ea) to head (44881df).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #184   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          204       218   +14     
  Branches       101       105    +4     
=========================================
+ Hits           204       218   +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JounQin]

Could all lines except if (expectedId !== id) be test covered?

[jedlikowski]

Could all lines except if (expectedId !== id) be test covered?

I've been trying to achieve that. AFAIK these are the only other lines not covered:

if (id < expectedId) {
  const waitingTime = Date.now() - start
  return receiveMessageWithId(
    port,
    expectedId,
    waitingTimeout ? waitingTimeout - waitingTime : undefined,
  )
}

Adding coverage for them in a stable manner would require us to mock receiveMessageOnPort so that it returns expectedId - 1 on first call and expectedId on second call. This is currently not possible in jest in ESM environment, because partial module mocking is not supported. Due to lack of support, it would be required to mock the whole node:worker_threads module 😞

[jedlikowski]

Good news @JounQin, I've managed add tests for those lines 😄 Could you take a look?

[jedlikowski]

Hi @JounQin, could you also add hacktoberfest label to this PR? 😄

[JounQin]

Thanks for your contribution!