fix: worker kept open when it shouldn't #119
Conversation
This closes #116
|
I've explained more about the fix here for reference #116 (comment) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #119 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 71 70 -1
Branches 16 16
=========================================
- Hits 71 70 -1 ☔ View full report in Codecov by Sentry. |
zakhenry
force-pushed
the
fix/worker-kept-open-when-it-should-not
branch
from
f57d27c to
91001ad
Compare
| @@ -69,7 +69,7 @@ export function fromWorkerPool<I, O>( | |||
| }), | |||
| map( | |||
| ([worker, unitWork]): Observable<O> => { | |||
| return fromWorker<I, O>(() => worker.factory(), of(unitWork), selectTransferables, { | |||
| return fromWorker<I, O>(() => worker.factory(), concat(of(unitWork), NEVER), selectTransferables, { | |||
There was a problem hiding this comment.
even though there's a test for it now, I think it's worth having a comment here explaining why the concat and never for the next time we read the code (prob in a long time and we won't remember why)
|
Please excuse my insistence, but I think you still need to change the |
|
@christian-schlichtherle it's a little more complicated than that, because with the worker interface we're not dealing with regular observables of values, but with observables of notifications. For this reason we don't expect to call .error or .complete because these signals are present as notifications, and the dematerialize at the end will convert that to the expected observable flow. I've tried out your fix and unfortunately it breaks the worker pool logic, so we can't accept that PR as-is. We're working on a fix but it might be a few days because this isn't our top priority right now. |
zakhenry
force-pushed
the
fix/worker-kept-open-when-it-should-not
branch
from
91001ad to
7a88a65
Compare
…in the pool context to keep the workers alive
zakhenry
force-pushed
the
fix/worker-kept-open-when-it-should-not
branch
from
7a88a65 to
8b68204
Compare
|
@christian-schlichtherle this PR is now ready I believe and should solve your issue. I'll wait for @maxime1992 to review (he is France timezone) and then we should be good to go. |
|
Tested and LGTM |
|
🎉 This PR is included in version 6.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This closes #116