endpoint: Try immediate send before queing pending #447
Conversation
src/controller/model/endpoint.ts
Outdated
| return await func(); | ||
| } catch(error) { | ||
| // TODO: Improve check for expired transaction | ||
| if (!sendWhenActive || error.code !== 240) { |
There was a problem hiding this comment.
I would not rely on the error code here, they might be different across different adapters
There was a problem hiding this comment.
Should we formalize an error type, or should we just throw it in pending regardless of the error when sendWhenActive is set?
There was a problem hiding this comment.
- sendWhenActive == false -> throw error
- sendWhenActive == true -> dont throw error
There was a problem hiding this comment.
Done, and expanded the tests to cover the new cases.
kennylevinsen
force-pushed
the
immediate-send
branch
from
238eefa
to
9f64f9f
Compare
src/controller/model/endpoint.ts
Outdated
| } else { | ||
| return func(); | ||
| private async sendRequest<Type>(func: () => Promise<Type>, sendWhenActive: boolean): Promise<Type> { | ||
| if (this.pendingRequests.length === 0) { |
There was a problem hiding this comment.
Why is the request only send when pendingRequests are empty? I would expect that a request with sendWhenActive = false would always be send directly.
There was a problem hiding this comment.
Hmm, true. I'll change that.
The idea was that if things had started queuing, we'd keep queuing for some amount of ordering, but it does behave a bit odd of you're asking for no queueing.
There was a problem hiding this comment.
Split off the queuing to avoid making things too ugly.
kennylevinsen
force-pushed
the
immediate-send
branch
from
9f64f9f
to
bdeabbd
Compare
|
Note: I'm currently missing test coverage, will get that sorted tomorrow. |
kennylevinsen
force-pushed
the
immediate-send
branch
2 times, most recently
from
ae33e0c
to
d9f8430
Compare
|
Test coverage fixed. Had some trouble getting the timing right in the test (sending data to wake things up after the request has been queued, but before we end up stalling on an await), but found a solution that's good enough. |
|
@kennylevinsen I've rebased it after #446 but now the coverage is failing, could you check this? |
sendWhenActive always queues the request for next time the pending queue was flushed. This has the unfortunate side-effect of delaying sends until next time the end device talks, even if the device is currently active. Instead, try to send the request immediately, and only fall back to the pending queue if that failed or if something was already queued.
kennylevinsen
force-pushed
the
immediate-send
branch
from
5618ea0
to
a1d38f2
Compare
|
Yeah, the new checkin test had to be adjusted slightly for the sendWhenActive behavior change. Fixed! |
|
Awesome, thanks! 🏅 |
sendWhenActive always queues the request for next time the pending queue
was flushed. This has the unfortunate side-effect of delaying sends
until next time the end device talks, even if the device is currently
active.
Instead, try to send the request immediately, and only fall back to the
pending queue if that failed or if something was already queued.
Notes: