eCAP

Body-related state and notification methods

Asked by Philip Allison

Hullo all - I think I may have found a number of bugs in the sample adapters, and even the VIGOS gzip adapter, which is rather worrying as I've only been looking at eCAP for a day! I also have some suggestions for how the API could be made easier to understand and less error-prone to code for, if my current understanding of it is correct.

Bug in all sample adapters, and VIGOS gzip adapter:

* They send noteAbContentDone as soon as they receive noteVbContentDone. This is dangerous, because the host may not have actually retrieved all adapted body from the adapter's buffer - it will only have done so if, every time the adapter sends noteAbContentAvailable, the host immediately consumes all data, but Squid's code caters for the case where it may consume less. Bearing this in mind, adapters should be written to assume hosts always call abContentShift eventually for data they have read, and send noteAbContentDone when abContentShift destroys the last data the adapter will ever send. If the adapter does not wait until the host has done its final read, then telling the host the VB content is "done" will cause the proxy to send incomplete data to the browser - hosts treat VbContentDone as authoritative, and stop sending, as verified by looking at XactionRep.cc in Squid!

* This raises an odd issue: since noteAbContentDone is a function call, not a return code, it will almost definitely be called *before* the host has returned from whatever function calls abContentShift.

Suggestions for improving the API if this is true:

* Why can the host ever perform a non-destructive read from the adapter? API would be far less error-prone if all the host could do was destructive reads, with an explicit EOF returned when the last data that is going to be produced is read. Also hosts should be coded to cope with the case where they are told data is available, then receive 0 bytes and an EOF, so that an adapter can signal that it's finished producing data in a way that works with non-blocking code. Think BSD sockets, but with EOF signalled explicitly, rather than implicitly by returning 0 bytes. noteAbContentDone must die!

* IMHO, noteVbContentDone should be removed in a similar way (force adapters to call vbContentShift and send EOF when done), although non-destructive reads for unshifted data are a good idea (useful, for example, for peeking at the first few bytes of body during streaming).

Second bug in sample adapters:

* Sample adapters should not call vbDiscard from abDiscard, as they may have already called vbMake in Xaction::start. If an adaptor has called vbMake, it should surely call vbStopMaking. Looking at the implementation of vbDiscard in Squid's XactionRep.cc, it will actually throw an exception if vbDiscard is called after vbMake.

Second suggestion for improving API: why do we need (vb|ab)Discard *and* (vb|ab)StopMaking? Surely it is trivial for hosts and adaptors to only delete body buffers if they're actually been allocated, and have a single call pair? I think this would be a better fit for the code the Squid team have actually written, because internally, its vbStopMaking and vbDiscard implementations end up calling the same function anyway! Also, it *never* calls abDiscard on adapters, only abStopMaking!

Please read my post and the sample code carefully, because I'm convinced I'm on to something here. If there is a valid reason for a host to ever perform a non-destructive read from the adapter, I would love to know what it is - but even if you want non-destructive reads to be possible, signalling that content is "done" should be a return code from a destructive read, in both halves of the API. This is how all other mainstream input/output APIs are implemented, both blocking and non-blocking. By not doing it this way, you're giving people too much rope to hang themselves - as shown by the presence of bugs in your own examples.

Regards,
Phil

Question information

Language:
English Edit question
Status:
Solved
For:
eCAP Edit question
Assignee:
No assignee Edit question
Solved by:
Alex Rousskov
Solved:
Last query:
Last reply:
Revision history for this message
Philip Allison (mangobrain) said : 		#1

Just noticed I mixed up my ABs and VBs a bit:

"If the adapter does not wait until the host has done its final read, then telling the host the VB content is "done" will cause the proxy to send incomplete data to the browser - hosts treat VbContentDone as authoritative, and stop sending, as verified by looking at XactionRep.cc in Squid!"

should be:

"If the adapter does not wait until the host has done its final read, then telling the host the AB content is "done" will cause the proxy to send incomplete data to the browser - hosts treat AbContentDone as authoritative, and stop sending, as verified by looking at XactionRep.cc in Squid!"

Revision history for this message
Philip Allison (mangobrain) said : 		#2

Further example to illustrate why noteVbContentDone/noteAbContentDone should be return codes and not function calls:

* Host has some data to give to the plugin. It so happens that this is the last chunk of data.
* Host calls noteVbContentAvailable. In response to this, the plugin calls noteAbContentAvailable (it's a very quick-acting plugin), and the host goes off and shifts some content off plugin's buffer. It so happens that this is the last chunk of data the host is going to produce, but the plugin daren't call noteAbContentDone, because it hasn't been told that it's been sent all of the VB.
* Host now calls noteVbContentDone. Plugin must be on its toes here, and test to see if its buffer is empty, calling noteAbContentDone if it is.

So the plugin must check for buffer emptiness in two places, and send noteAbContentDone from one of two places.

If we had return codes on ContentShift:

* Host has some data to give to plugin. It calls noteVbContentAvailable.
* Plugin retrieves the data, calls vbContentShift, and gets told it is the last chunk.
* Plugin calls noteAbContentAvailable.
* Host retrieves the data, calls abContentShift (should be implicit because *why does the host need non-destructive reads*), and gets told it is the last chunk.

How much simpler is that? :)

Revision history for this message
Best Alex Rousskov (rousskov) said : 		#3

Hello Phil,

    Thank you very much for a thoughtful post. I will try to extract and answer your questions. Please let me know if I missed any.

* Is it OK to call Host::Xaction::noteAbContentDone as soon as the adapter receives noteVbContentDone?

Yes, it is. In general, noteAbContentDone should be called when the adapter is done producing the adapted content.

* Is it possible that Host::Xaction::noteAbContentDone is called before the host had a chance to consume the entire adapted content?

Yes, it is. It is the responsibility of the host application to handle such situations. This is no different than, for example, a proxy receiving the end of an HTTP response from the origin server while still having a lot of data to write to the HTTP client.

Under normal conditions, after receiving noteAbContentDone, the host should keep using the adapted content until all such content is used up. The notification informs the host about the change in the producer (adapter) state. It does not mandate any specific actions from the consumer (host).

* Does Squid implement Host::Xaction::noteAbContentDone correctly?

No, it does not (as of 2009/03/03). You are correct that Squid should not call BodyPipe::stopProducingFor() if there is still unconsumed adapted body present in the transaction. This bug needs to be filed with the Squid project and fixed.

* Should hosts always call Adapter::Xaction::abContentShift for adaptation data they no longer need?

Yes, they should. Calling abContentShift may trigger production of more adapted body data, for example, and there could be other side effects and consequences. This is unrelated to sending noteAbContentDone though.

* Is Host::Xaction::noteAbContentDone always called before the host has returned from Adapter::Xaction::abContentShift?

No. The calls could (and probably should) be asynchronous. If the host does not use asynchronous calls, it is the host responsibility to handle "reentrant" calls.

* Are all body reads non-destructive?

Yes, they are, assuming a "destructive read" means copying body data from buffer A into some buffer B and forgetting about the copied data in A during the same function call. The eCAP API allows the agents to mimic destructive reads but does not require them.

* Why are all body reads non-destructive?

To simplify "zero-copy" implementation of non-modifying transactions (a common case) and to simplify implementation of asynchronous communication between the host and the adapter.

Also, some adapters may optimize by knowing that there is no more body content coming, even before they consumed the already available content.

* Do eCAP implementation bugs illustrate that all reads should be "destructive" and return EOF when there is nothing more to read?

No more than bugs and inefficiencies in applications using "destructive" reads illustrate that the opposite is true :-).

The eCAP API can and should be improved, but I doubt that making destructive reads mandatory and delaying EOF information is the best direction (for reasons outlined in other answers).

One interesting idea would be to provide an agent method implementing a destructive read with EOF return code, using the existing non-destructive interfaces. Do you think the eCAP library can and should do that?

* Should the Modifying Adapter sample call vbDiscard from abDiscard and abStopMaking?

No, it should not. This is a bug. This particular adapter should call vbStopMaking and change receivingVb state instead to tell the host that the adapter is not interested in the rest of the virgin body if the host is not interested in the adapted body. The fix has been committed and will be available in the next sample release. I will try to attach the patch to this question or will file a bug report.

Other adapters may need to call vbDiscard, depending on their state.

* Do we need both (vb|ab)Discard and (vb|ab)StopMaking?

The two methods have distinct semantics. Having two methods makes more state checks possible. However, I agree that these reasons may not be sufficient to justify having both methods. I will probably remove one and rename the other. Can you think of a good replacement method name?

* Is this the right place to discuss or report bugs in 3rd party eCAP adapters such as VIGOS gzip adapter?

No, it is not. Please contact the authors of the corresponding adapter code. Only official eCAP adapters such as sample adapters are covered by this site.

In the future, please try to post one primary question per Question to make it easier for other folks to find the answers they are looking for. It also makes it possible to convert a Question into a bug report if needed. And please report bugs using the Bugs tab. It is easier to track bugs state there.

Thank you,

Alex.

Revision history for this message
Alex Rousskov (rousskov) said : 		#4

I may have found one more question in your follow up post:

* Does the eCAP API assume some specific relationship between virgin and adapted bodies?

No, it does not. An adapter may

- Receive the virgin body without ever producing an adapted body.
- Produce an adapted body without ever receiving the virgin body.
- Produce an adapted body before, during, or after receiving the virgin body.

When both virgin and adapted body are in play, eCAP does not assume that they are, somehow, related. There are existing adapters that generate adapted bodies that are pretty much unrelated to the virgin bodies, yet some of those adapters still want to receive virgin bodies to perform statistical analysis and other tricks that have little effect on a given adapted body production.

Moreover, the adapter behavior may change from one transaction to another.

Varying adapter needs make it impossible to hard-code body termination steps into the library even though some specific adapters will always want to stop producing the adapted body immediately after they have fully received and processed the virgin body.

Revision history for this message
Alex Rousskov (rousskov) said : 		#5

BTW, basic POSIX I/O methods do not return EOF status when you read the last chunk of data. If there is some data in the buffer, you have to call read twice to know that there is no more coming. If that is the interface you prefer, you can mimic it using eCAP, right? Just write a "destructive" adapter read() method that:

- If there is virgin content, copies and consumes it, returning consumed size.
- If there is no virgin content but more is expected, returns an EAGAIN or some such.
- Otherwise, returns zero.

The above approach will give you the interface you seem to prefer without requiring others to always copy and consume content (which can be expensive). You can even improve it by adding a EOF flag as an output parameter so that you do not have to read twice to notice EOF.

The question is whether the library itself should provide such a wrapper or, perhaps, keep track of the EOF state.

Thank you,

Alex.

Revision history for this message
Philip Allison (mangobrain) said : 		#6

Sorry for posting so much in one message - I have just been taking a first look at eCAP to evaluate it as a possible technology for a future web filtering project, and wasn't sure if what I had found were bugs or just misunderstandings of the API, so I didn't want to jump straight to the "bugs" tab prematurely. The sooner we can have some documentation, the better, but that's a separate issue ;)

Thanks for taking the time to read through my comments, and for your helpful and receptive response. Much appreciated.

I will file bugs against the sample adaptors unless you'd rather do it yourself. I will also contact the Squid and VIGOS guys about the possible bugs in their code - might even have a stab at patching Squid myself, as their eCAP code is fairly readable once one understands the eCAP API.

You've clarified a lot of the things I was wondering about, but I'd still like to continue this discussion - is this the best place, or is there a mailing list or some other better-suited place for it to go?

Revision history for this message
Philip Allison (mangobrain) said : 		#7

Thanks Alex Rousskov, that solved my question.