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.

Sign up for GitHub

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

Add 5s timeout for market operations #305

Merged
merged 2 commits into from
Mar 31, 2021
Merged

Add 5s timeout for market operations #305

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5050bf
Add 5s timeout for market operations
azawlocki Mar 30, 2021
9a0af0f
Simplify the logic in find_offers() slightly; fix rejection message
azawlocki Mar 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 37 additions & 22 deletions yapapi/executor/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,32 @@ async def accept_payment_for_agreement(agreement_id: str, *, partial: bool = Fal
)

async def find_offers() -> None:
async def reject_proposal(proposal, reason):
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm... the only thing I'm concerned about is that again, we're adding inner-scope functions ... whereas maybe we should start pulling that stuff outside ... but I do understand that the milk is spilled by find_offers already so it would be artificial to pull these as methods and leave find_offers as internal...

Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes inner functions are just better - object-oriented programming is not the only paradigm, let's not be afraid of them. Actually, I like @azawlocki's solution very much, it's an improvement over the original code and over my PR to yajsapi, which AFAIR added another copy of try/call api/catch/emit.

Copy link
Contributor

Choose a reason for hiding this comment

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

These functions are not visible outside of find_offers(). If they are not used outside, IMO they shouldn't be moved as they are "helper" functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shadeofblue I've just started pulling stuff outside in #306, I'm not sure we want to do that kind of refactoring in a PR to a release branch.

try:
await proposal.reject(reason=reason)
emit(events.ProposalRejected(prop_id=proposal.id, reason=reason))
return True
except Exception:
emit(
events.ProposalFailed(
prop_id=proposal.id, exc_info=sys.exc_info() # type: ignore
)
)
return False

async def respond_to_proposal(proposal, builder):
try:
await proposal.respond(builder.properties, builder.constraints)
emit(events.ProposalResponded(prop_id=proposal.id))
return True
except Exception:
emit(
events.ProposalFailed(
prop_id=proposal.id, exc_info=sys.exc_info() # type: ignore
)
)
return False

nonlocal offers_collected, proposals_confirmed
try:
subscription = await builder.subscribe(market_api)
Expand All @@ -394,12 +420,10 @@ async def find_offers() -> None:
score,
)
except InvalidPropertiesError as err:
emit(events.ProposalRejected(prop_id=proposal.id, reason=str(err)))
await reject_proposal(proposal, "Malformed offer")
continue
if score < SCORE_NEUTRAL:
with contextlib.suppress(Exception):
await proposal.reject(reason="Score too low")
emit(events.ProposalRejected(prop_id=proposal.id, reason="Score too low"))
await reject_proposal(proposal, "Score too low")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not required, because there is no code after all if branches, but maybe we can add continue after the call to show our intent? Feel free to ignore this suggestion.

Suggested change
await reject_proposal(proposal, "Score too low")
await reject_proposal(proposal, "Score too low")
continue

elif not proposal.is_draft:
try:
common_platforms = self._get_common_payment_platforms(proposal)
Expand All @@ -409,31 +433,22 @@ async def find_offers() -> None:
)
else:
# reject proposal if there are no common payment platforms
with contextlib.suppress(Exception):
await proposal.reject(reason="No common payment platform")
emit(
events.ProposalRejected(
prop_id=proposal.id, reason="No common payment platforms"
)
)
await reject_proposal(proposal, "No common payment platform")
continue
timeout = proposal.props.get(DEBIT_NOTE_ACCEPTANCE_TIMEOUT_PROP)
if timeout:
if timeout < DEBIT_NOTE_MIN_TIMEOUT:
with contextlib.suppress(Exception):
await proposal.reject(reason="Debit note timeout too low")
emit(
events.ProposalRejected(
prop_id=proposal.id,
reason="Debit note acceptance timeout too short",
)
)
await reject_proposal(proposal, "No common payment platform")
azawlocki marked this conversation as resolved.
Show resolved Hide resolved
continue
else:
builder.properties[DEBIT_NOTE_ACCEPTANCE_TIMEOUT_PROP] = timeout
await proposal.respond(builder.properties, builder.constraints)
emit(events.ProposalResponded(prop_id=proposal.id))
if await respond_to_proposal(proposal, builder):
emit(events.ProposalResponded(prop_id=proposal.id))
else:
continue
except CancelledError:
raise
except Exception as ex:
except Exception:
emit(
events.ProposalFailed(
prop_id=proposal.id, exc_info=sys.exc_info() # type: ignore
Expand Down
4 changes: 2 additions & 2 deletions yapapi/rest/market.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,14 @@ def is_draft(self) -> bool:
async def reject(self, reason: str = "Rejected"):
"""Reject the Offer."""
await self._subscription._api.reject_proposal_offer(
self._subscription.id, self.id, request_body={"message": reason}
self._subscription.id, self.id, request_body={"message": reason}, _request_timeout=5
)

async def respond(self, props: dict, constraints: str) -> str:
"""Create an agreeement Proposal for a received Offer, based on our Demand."""
proposal = models.DemandOfferBase(properties=props, constraints=constraints)
new_proposal = await self._subscription._api.counter_proposal_demand(
self._subscription.id, self.id, proposal
self._subscription.id, self.id, proposal, _request_timeout=5
)
return new_proposal

Expand Down