Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix enqueue race condition. #107

Merged
merged 1 commit into from Sep 15, 2016
Merged

Fix enqueue race condition. #107

merged 1 commit into from Sep 15, 2016

Conversation

brycedrennan
Copy link
Contributor

@brycedrennan brycedrennan commented Sep 14, 2016

Sometimes emails in the queue will already have been processed before the first attempt here.

I discovered this when our full integration test was showing duplicate emails sent. This fixed the issue.

This is basically the same issue as #94

@coveralls
Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage decreased (-0.05%) to 93.613% when pulling 2f3d233 on CircleUp:enqueue-race into aced65a on slimta:master.

@@ -329,8 +329,9 @@ def enqueue(self, envelope):
for env, id in results:
if not isinstance(id, BaseException):
if self.relay:
self.active_ids.add(id)
self._pool_spawn('relay', self._attempt, id, env, 0)
if id not in self.active_ids:
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick, but would you collapse this if into one?

if self.relay and id not in self.active_ids:
    # ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@coveralls
Copy link

coveralls commented Sep 15, 2016

Coverage Status

Coverage decreased (-0.2%) to 93.5% when pulling 19a467b on CircleUp:enqueue-race into aced65a on slimta:master.

self._pool_spawn('relay', self._attempt, id, env, 0)
if self.relay and id not in self.active_ids:
self.active_ids.add(id)
self._pool_spawn('relay', self._attempt, id, env, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry one more thing, pull back the indentation one more level on these two lines and you'll be good to go!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops my bad. fixed.

Sometimes emails in the queue will already have been processed before the first attempt here.
Copy link
Member

@icgood icgood left a comment

Choose a reason for hiding this comment

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

lgtm, will merge!

@coveralls
Copy link

coveralls commented Sep 15, 2016

Coverage Status

Coverage remained the same at 93.667% when pulling d1b076e on CircleUp:enqueue-race into aced65a on slimta:master.

@icgood icgood merged commit b0e03a1 into slimta:master Sep 15, 2016
@icgood icgood added this to the 3.3 milestone Sep 15, 2016
@icgood
Copy link
Member

icgood commented Sep 15, 2016

@brycedrennan just tagged and uploaded version 3.2.3 which contains this fix. Thanks!

@brycedrennan
Copy link
Contributor Author

wow that's great! will make our deploy easier

@brycedrennan brycedrennan deleted the enqueue-race branch September 15, 2016 17:25
@icgood icgood modified the milestones: 3.3, 4.0 Nov 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants