feat(pcapgo): support cancellation with context.Context

[prskr]

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.Context but I also optimized some small things to avoid allocations in hot paths:

• re-order the IPv4 header parsing
• NewZeroCopyPacketSource

I'm using these changes already for quite some time so I'm 'optimistic' they're working as expected 😄

[mosajjal]

Haven't read through it yet (on the phone) how much overhead doea this add?

[prskr]

What do you mean how much overhead does this add?

Edit: the context.Context support basically nothing. It's only used in a single loop which was previously unconditionally 😅

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

[prskr]

I just realized I might've missed one additional improvement for the context.Context support to unlock the read operation and directly return. Will check that tomorrow 😊

[mosajjal]

wondering if this creates any limitation for packets in lo or jumboframes?

[prskr]

that's actually the previous value only in a constant instead of having it hard coded 😄

[mosajjal]

from what I understand, the API for packetReader won't change. eg any existing code should work with context.Background(). is that right?

[mosajjal]

in my opinon, if we need to have the netlink dependency, we should make it clear that it must stay contained within testing.

[prskr]

sure, although it's not new, it's in the go.mod since June 21 😅

[prskr]

Yes, that's right. No public API is touched.

It could make sense to either make a breaking change or introduce similar overloads for:

• func (p *PacketSource) NextPacket() (Packet, error) like func (p *PacketSource) NextPacketCtx(ctx context.Context) (Packet, error)
• ZeroCopyReadPacketData() (data []byte, ci CaptureInfo, err error) to ZeroCopyReadPacketDataCtx(ctx context.Context) (data []byte, ci CaptureInfo, err error)
• ReadPacketData() (data []byte, ci CaptureInfo, err error) to ReadPacketDataCtx(ctx context.Context) (data []byte, ci CaptureInfo, err error)

to be able to unblock these routines as well but not really sure how to proceed with that

[mosajjal]

for the time being, not touching the API is a better call. is this ready for merge?

[prskr]

Not a problem, just wanted to mention it 😄
If you don't have any further questions/objections I'd say this is ready, yes 😄
Tests are passing (also the one in my project where I'm already using my fork) so from my side I'm all set 😊

[mosajjal]

yep tests are passing so hopefully nothing will break :) if it does, we need better tests :P