Inconsistent behaviour when cleaning up plum

[oviano]

When my application quits it calls destroy mapping on two mappings it created earlier and then calls plum_cleanup.

With UPnP it waits for the pending http requests to complete before the client exits.

With PCP it does not, and the responses to the unmappings get mixed up.

I think it should be possible to clean up plum so that it waits for the usual timeouts etc when unmapping PCP.

This could be achieved by adding the "hard" flag to plum_cleanup() and passing it down the library so that the client interruption can optionally be "soft".

[paullouisageneau]

If the two responses get mixed up it sounds more like a bug in the PCP implementation. I think the whole concept of hard and soft interruptions is a bit shaky because hard interruptions end up creating this kind of corner cases, because in PCP there is no way to map responses to requests. I think it could make more sense to make it behaving like soft interrupt all the time, which would solve this issue. Is it correct?

[oviano]

If the two responses get mixed up it sounds more like a bug in the PCP implementation.

The sequence of events was something like this:

• two mappings created successfully
• app runs for a while
• two mappings requested to be destroyed one immediately after the other
• first mapping starts process of deleting, i..e sending UDP unmap request and waiting for response
• clean up called
• this interrupts the wait for the first unmap response
• code sees that another mapping is to be destroyed so defers client shutdown
• second mapping starts process of deleting, i.e. sending UDP unmap request and waiting for response
• response to first unmap received by second wait loop
• at some point another response comes in with an error code of 8
• client completes shutdown

If I enforce a "soft" shutdown, no errors occur.

I think it could make more sense to make it behaving like soft interrupt all the time, which would solve this issue. Is it correct?

Yes. The timeouts on PCP seem pretty small, though in theory it looks like they start small (250ms) and double, so potentially if there was a network issue then it could delay shutdown. For me, that's not a problem, I'd rather a proper attempt was made to unmap.

[oviano]

If you are ok with all cleanups being "soft" then from what I can see you would effectively be replacing the enums used by UPnP and PCP...

typedef enum { UPNP_INTERRUPT_NONE, UPNP_INTERRUPT_SOFT, UPNP_INTERRUPT_HARD } upnp_interrupt_t;

...with a bool instead, but used atomically in the same way as the above.

As from what I can tell, the only time a hard interrupt is used is on client shutdown.

If you agree, I can make a PR, but maybe you want to keep the concept of a soft/hard interrupt in the code, but for now just only use the soft interrupt on clean up?

[paullouisageneau]

You are right, but for now you can simply use the soft interrupt on clean up. The hard interrupt might still be useful for a destroy immediately without cleanup API.

[oviano]

https://github.com/paullouisageneau/libplum/pull/38