Dispose methods and finally blocks don't throw exceptions.#767
Open
aidmsu wants to merge 4 commits intoHangfireIO:devfrom
aidmsu:unthrowable-dispose
Open
Dispose methods and finally blocks don't throw exceptions.#767aidmsu wants to merge 4 commits intoHangfireIO:devfrom aidmsu:unthrowable-dispose
aidmsu wants to merge 4 commits intoHangfireIO:devfrom
aidmsu:unthrowable-dispose
Conversation
Current coverage is 67.73% (diff: 68.75%)
|
odinserj
requested changes
Dec 21, 2016
Member
odinserj
left a comment
odinserj
left a comment
There was a problem hiding this comment.
Thanks for submitting this PR, I've added some comments.
| /// <param name="properties"></param> | ||
| /// <param name="options"></param> | ||
| public BackgroundProcessingServer( | ||
| [NotNull] JobStorage storage, |
Member
There was a problem hiding this comment.
First, please remove this change...
| [NotNull] JobStorage storage, | ||
| [NotNull] JobStorage storage, | ||
| [NotNull] IEnumerable<IBackgroundProcess> processes, | ||
| [NotNull] IDictionary<string, object> properties, |
|
|
||
| var context = new BackgroundProcessContext( | ||
| GetGloballyUniqueServerId(), | ||
| GetGloballyUniqueServerId(), |
|
|
||
| private const string DistributedLockKey = "locks:expirationmanager"; | ||
| private static readonly TimeSpan DefaultLockTimeout = TimeSpan.FromMinutes(5); | ||
| // Note, that lock escalation may also happen during the cascade deletions for | ||
| // State (3-5 rows/job usually) and JobParameters (2-3 rows/job usually) tables. | ||
| private const int NumberOfRecordsInSinglePass = 1000; | ||
| if ((shutdownTimeout < TimeSpan.Zero && shutdownTimeout != Timeout.InfiniteTimeSpan) || | ||
| shutdownTimeout.TotalMilliseconds > Int32.MaxValue) | ||
| { | ||
| Logger.Warn($@"ShutdownTimeout equals {_options.ShutdownTimeout.Milliseconds} milliseconds and it't incorret. |
| catch (Exception ex) | ||
| { | ||
| connection.RemoveServer(context.ServerId); | ||
| Logger.WarnException($@"Couldn't remove server {context.ServerId}. The server can be displayed on 'Server' page of Dashboard for a while. |
Member
There was a problem hiding this comment.
The server phrase duplication, you can replace it with the , but it won't perform.
| // Timer callback may be invoked after the Dispose method call, | ||
| // so we are using lock to avoid unsynchronized calls. | ||
|
|
||
| try |
Member
There was a problem hiding this comment.
Why did you remove the try-catch statement? The absence of the SqlException does not guarantee the absence of any other exceptions, like ThreadAbortedException, etc. The method can still throw, and we still should release the acquired connection.
|
|
||
| internal static void Release(IDbConnection connection, string resource) | ||
| { | ||
| var couldNotReleaseLockMessageTemplate = $@"Could not release a lock on the resource '{resource}'. {0} |
Member
There was a problem hiding this comment.
Please use single line for log messages.
| } | ||
| catch (SqlException ex) | ||
| { | ||
| Logger.WarnException(string.Format(couldNotReleaseLockMessageTemplate, string.Empty), ex); |
Member
There was a problem hiding this comment.
string.Empty may cause extra spaces in log message.
odinserj
force-pushed
the
dev
branch
2 times, most recently
from
2183dda to
5ab7e98
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dispose methods and finally blocks don't throw any exceptions. Instead exceptions are caught and logged.