feat(pcapgo): support cancellation with context.Context#12
Conversation
2b73dc4 to
d4f7dea
Compare
|
Haven't read through it yet (on the phone) how much overhead doea this add? |
|
What do you mean how much overhead does this add? Edit: the There was an issue of properly closing a packet source which didn't unlock the reading goroutine I resolved. The re-ordering I honestly can't remember but I also don't think it really hurts because it basically only makes sure all the error cases are handled before the remaining information is extracted. Regarding the 'zero allocation' + pooled packets: this was a huge improvement in my experience. I used to work in a company which is recording PCAPs all day long for analytics and it improved the overall memory consumption 10x so roughly from 250-300MB to 25-40MB |
|
I just realized I might've missed one additional improvement for the |
| "time" | ||
| ) | ||
|
|
||
| const maximumMTU = 1500 |
There was a problem hiding this comment.
wondering if this creates any limitation for packets in lo or jumboframes?
There was a problem hiding this comment.
that's actually the previous value only in a constant instead of having it hard coded 😄
mosajjal
left a comment
There was a problem hiding this comment.
from what I understand, the API for packetReader won't change. eg any existing code should work with context.Background(). is that right?
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/vishvananda/netlink" |
There was a problem hiding this comment.
in my opinon, if we need to have the netlink dependency, we should make it clear that it must stay contained within testing.
There was a problem hiding this comment.
sure, although it's not new, it's in the go.mod since June 21 😅
|
Yes, that's right. No public API is touched. It could make sense to either make a breaking change or introduce similar overloads for:
to be able to unblock these routines as well but not really sure how to proceed with that |
|
for the time being, not touching the API is a better call. is this ready for merge? |
|
Not a problem, just wanted to mention it 😄 |
|
yep tests are passing so hopefully nothing will break :) if it does, we need better tests :P |
I originally prepared this PR for the google/gopacket repo.
Although it's already quite a while I finally managed to re-create it to (hopefully) merge it now 😄
It's mostly about adding support for
context.Contextbut I also optimized some small things to avoid allocations in hot paths:NewZeroCopyPacketSourceI'm using these changes already for quite some time so I'm 'optimistic' they're working as expected 😄