fix: Prevent exception on return of job ID to disposed pool #48
Conversation
|
@gfoidl No need to hurry, hotfixed the previous release, because there's some unreleased changes on main anyway. |
…eption-on-dispose
|
Thanks @gfoidl, appreciate your opinion a lot. I intend to add some unit tests for this behavior as well, so I'm not merging straight away. |
|
|
||
| public ValueTask<int> RentJobIdAsync(CancellationToken cancellationToken) => _jobIdPool.Reader.ReadAsync(cancellationToken); | ||
|
|
||
| public void ReturnJobId(int jobId) | ||
| { | ||
| if (!_jobIdPool.Writer.TryWrite(jobId)) | ||
| if (!_jobIdPool.Writer.TryWrite(jobId) && !Volatile.Read(ref _disposed)) |
There was a problem hiding this comment.
Is there a reason for the ordering? Wouldn't you want the !Volatile.Read(ref _disposed) checked first, and the TryWrite only executed if it isn't yet disposed, taking care of short-circuiting &&.
Or is there a reason for always executing the writing to the jobIdPool, independent of it being disposed or not?
There was a problem hiding this comment.
TryWrite is called more often than Dispose so I think that order is fine, meaning that the common path comes first.
If TryWrite fails, then it's checked if disposed or not.
But TBH I don't think that any order here actually matters. The arguing of @scamille is also fine.
Both yield the same result, and perf-wise there's no difference (at least with TCP connection to PLC in mind). And I have no problem if the order is flipped.
There was a problem hiding this comment.
There's also the hypothetical race condition of _disposed not yet being set when it's read, but the TryWrite failing because the writer had been Complete()-ed in the meantime (assuming the Volatile.Read would come first). I'm not sure if there's anything in .NET that rules out this race condition, but this ordering should prevent it either way.
mycroes
force-pushed
the
job-pool-exception-on-dispose
branch
from
9b237ba to
4b335b0
Compare
Currently when the connection is disposed and requests are still in the queue they will except because the job ID can no longer be returned to the pool. Making the exception on
ReturnJobIdconditional on a new_disposedflag prevents the exception from being thrown, but it does permit 'successful' returns to a disposed pool.@gfoidl please let me know what you think. This is hitting us in production now, so I'll probably merge this quickly.